Remove iterator interface from LBPolicy Metadata (#26492)

* Remove iterator interface from LBPolicy Metadata

Trying to clear the way to think about how metadata is handled in core.

Right now this interface is only used for tests, so a single method that
does what's needed for that (and marked TestOnly) is provided for now.

In the future we'll certainly likely need API to access metadata from
load balancing policies. However, it seems likely that we'll want to
encourage access via lookup-by-key rather than lookup-by-iteration, and
so it seems likely we'll want to expose interfaces phrased in those
terms.

In the meantime, this change localizes some complexity to make it easier
to transition to new internal API's.

* mdcleanup

src/core/ext/filters/client_channel/client_channel.cc

@@ -2501,48 +2501,21 @@ class ClientChannel::LoadBalancedCall::Metadata
                GRPC_ERROR_NONE);
   }
 
-  iterator begin() const override {
-    static_assert(sizeof(grpc_linked_mdelem*) <= sizeof(intptr_t),
-                  "iterator size too large");
-    return iterator(
-        this, reinterpret_cast<intptr_t>(MaybeSkipEntry(batch_->list.head)));
+  std::vector<std::pair<std::string, std::string>> TestOnlyCopyToVector()
+      override {
+    std::vector<std::pair<std::string, std::string>> result;
+    for (grpc_linked_mdelem* entry = batch_->list.head; entry != nullptr;
+         entry = entry->next) {
+      if (batch_->idx.named.path != entry) {
+        result.push_back(std::make_pair(
+            std::string(StringViewFromSlice(GRPC_MDKEY(entry->md))),
+            std::string(StringViewFromSlice(GRPC_MDVALUE(entry->md)))));
       }
-  iterator end() const override {
-    static_assert(sizeof(grpc_linked_mdelem*) <= sizeof(intptr_t),
-                  "iterator size too large");
-    return iterator(this, 0);
     }
-
-  iterator erase(iterator it) override {
-    grpc_linked_mdelem* linked_mdelem =
-        reinterpret_cast<grpc_linked_mdelem*>(GetIteratorHandle(it));
-    intptr_t handle = reinterpret_cast<intptr_t>(linked_mdelem->next);
-    grpc_metadata_batch_remove(batch_, linked_mdelem);
-    return iterator(this, handle);
+    return result;
   }
 
  private:
-  grpc_linked_mdelem* MaybeSkipEntry(grpc_linked_mdelem* entry) const {
-    if (entry != nullptr && batch_->idx.named.path == entry) {
-      return entry->next;
-    }
-    return entry;
-  }
-
-  intptr_t IteratorHandleNext(intptr_t handle) const override {
-    grpc_linked_mdelem* linked_mdelem =
-        reinterpret_cast<grpc_linked_mdelem*>(handle);
-    return reinterpret_cast<intptr_t>(MaybeSkipEntry(linked_mdelem->next));
-  }
-
-  std::pair<absl::string_view, absl::string_view> IteratorHandleGet(
-      intptr_t handle) const override {
-    grpc_linked_mdelem* linked_mdelem =
-        reinterpret_cast<grpc_linked_mdelem*>(handle);
-    return std::make_pair(StringViewFromSlice(GRPC_MDKEY(linked_mdelem->md)),
-                          StringViewFromSlice(GRPC_MDVALUE(linked_mdelem->md)));
-  }
-
   LoadBalancedCall* lb_call_;
   grpc_metadata_batch* batch_;
 };

src/core/ext/filters/client_channel/lb_policy.h

@@ -134,35 +134,16 @@ class LoadBalancingPolicy : public InternallyRefCounted<LoadBalancingPolicy> {
   /// Implemented by the client channel and used by the SubchannelPicker.
   class MetadataInterface {
    public:
-    class iterator
-        : public std::iterator<
-              std::input_iterator_tag,
-              std::pair<absl::string_view, absl::string_view>,  // value_type
-              std::ptrdiff_t,  // difference_type
-              std::pair<absl::string_view, absl::string_view>*,  // pointer
-              std::pair<absl::string_view, absl::string_view>&   // reference
-              > {
-     public:
-      iterator(const MetadataInterface* md, intptr_t handle)
-          : md_(md), handle_(handle) {}
-      iterator& operator++() {
-        handle_ = md_->IteratorHandleNext(handle_);
-        return *this;
-      }
-      bool operator==(iterator other) const {
-        return md_ == other.md_ && handle_ == other.handle_;
-      }
-      bool operator!=(iterator other) const { return !(*this == other); }
-      value_type operator*() const { return md_->IteratorHandleGet(handle_); }
-
-     private:
-      friend class MetadataInterface;
-      const MetadataInterface* md_;
-      intptr_t handle_;
-    };
-
     virtual ~MetadataInterface() = default;
 
+    //////////////////////////////////////////////////////////////////////////
+    // TODO(ctiller): DO NOT MAKE THIS A PUBLIC API YET
+    // This needs some API design to ensure we can add/remove/replace metadata
+    // keys... we're deliberately not doing so to save some time whilst
+    // cleaning up the internal metadata representation, but we should add
+    // something back before making this a public API.
+    //////////////////////////////////////////////////////////////////////////
+
     /// Adds a key/value pair.
     /// Does NOT take ownership of \a key or \a value.
     /// Implementations must ensure that the key and value remain alive
@@ -170,23 +151,9 @@ class LoadBalancingPolicy : public InternallyRefCounted<LoadBalancingPolicy> {
     /// CallState::Alloc().
     virtual void Add(absl::string_view key, absl::string_view value) = 0;
 
-    /// Iteration interface.
-    virtual iterator begin() const = 0;
-    virtual iterator end() const = 0;
-
-    /// Removes the element pointed to by \a it.
-    /// Returns an iterator pointing to the next element.
-    virtual iterator erase(iterator it) = 0;
-
-   protected:
-    intptr_t GetIteratorHandle(const iterator& it) const { return it.handle_; }
-
-   private:
-    friend class iterator;
-
-    virtual intptr_t IteratorHandleNext(intptr_t handle) const = 0;
-    virtual std::pair<absl::string_view /*key*/, absl::string_view /*value */>
-    IteratorHandleGet(intptr_t handle) const = 0;
+    /// Produce a vector of metadata key/value strings for tests.
+    virtual std::vector<std::pair<std::string, std::string>>
+    TestOnlyCopyToVector() = 0;
   };
 
   /// Arguments used when picking a subchannel for a call.

test/core/util/test_lb_policies.cc

@@ -76,19 +76,6 @@ class ForwardingLoadBalancingPolicy : public LoadBalancingPolicy {
   OrphanablePtr<LoadBalancingPolicy> delegate_;
 };
 
-//
-// CopyMetadataToVector()
-//
-
-MetadataVector CopyMetadataToVector(
-    LoadBalancingPolicy::MetadataInterface* metadata) {
-  MetadataVector result;
-  for (auto p : *metadata) {
-    result.push_back({std::string(p.first), std::string(p.second)});
-  }
-  return result;
-}
-
 //
 // TestPickArgsLb
 //
@@ -119,7 +106,7 @@ class TestPickArgsLb : public ForwardingLoadBalancingPolicy {
       // Report args seen.
       PickArgsSeen args_seen;
       args_seen.path = std::string(args.path);
-      args_seen.metadata = CopyMetadataToVector(args.initial_metadata);
+      args_seen.metadata = args.initial_metadata->TestOnlyCopyToVector();
       cb_(args_seen);
       // Do pick.
       return delegate_picker_->Pick(args);
@@ -293,7 +280,7 @@ class InterceptRecvTrailingMetadataLoadBalancingPolicy
       TrailingMetadataArgsSeen args_seen;
       args_seen.backend_metric_data = call_state->GetBackendMetricData();
       GPR_ASSERT(recv_trailing_metadata != nullptr);
-      args_seen.metadata = CopyMetadataToVector(recv_trailing_metadata);
+      args_seen.metadata = recv_trailing_metadata->TestOnlyCopyToVector();
       cb_(args_seen);
       this->~TrailingMetadataHandler();
     }