This looks like a good start. Please let me know if you have any questions about anything.

We should probably also have someone from grpclb-team review this.

Reviewed 5 of 16 files at r1, 11 of 11 files at r2, 5 of 5 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

build.yaml, line 1880 at r3 (raw file):

  deps:
  - interop_server_lib
- name: lb_load_data_store

These targets also need to be added to the BUILD files.

src/cpp/server/load_reporter/load_data_store.h, line 31 at r2 (raw file):

namespace grpc {

class CallMetricValue {

Please document all classes and all non-trivial methods.

It might also be useful to have a top-level comment here explaining the high-level structure here (i.e., identify all data structures and their relationships to each other).

src/cpp/server/load_reporter/load_data_store.h, line 50 at r2 (raw file):

};

class Key {

This name is much too generic for the grpc namespace. I suggest changing it to something like LoadReportKey. Alternatively, consider moving all of this code into a sub-namespace called load_reporter.

src/cpp/server/load_reporter/load_data_store.h, line 87 at r2 (raw file):

  // To use Hasher.
  friend class PerBalancerStore;

Please avoid using friend whenever possible (other than for testing purposes). In this case, it seems fine to just make Hasher a public member.

src/cpp/server/load_reporter/load_data_store.h, line 94 at r2 (raw file):

};

class Value {

Same as above: this name is too generic for this namespace.

src/cpp/server/load_reporter/load_data_store.h, line 135 at r2 (raw file):

           ", bytes_sent_=" + grpc::to_string(bytes_sent_) +
           ", "
           "bytes_recv_=" +

Nit: This can be merged with the string on the previous line.

src/cpp/server/load_reporter/load_data_store.h, line 221 at r2 (raw file):

 private:
  friend class LoadDataStore;

Why is this needed? Can't we just make the methods used by LoadDataStore public?

src/cpp/server/load_reporter/load_data_store.h, line 233 at r2 (raw file):

  // Key: LB ID. The key set includes all the LB IDs that have been
  // allocated for reporting streams so far.
  std::unordered_map<grpc::string, PerBalancerStore*> per_balancer_stores_;

How about making the value a std::unique_ptr<>, so that the dtor doesn't need to explicitly delete the values?

src/cpp/server/load_reporter/load_data_store.cc, line 70 at r2 (raw file):

PerHostStore::~PerHostStore() {
  for (auto it_per_balancer_store : per_balancer_stores_) {

When using auto in a for loop, I suggest changing it to auto&, to ensure that we don't make an unnecessary copy. Also suggest using const auto& whenever possible.

Same thing throughout this PR.

src/cpp/server/load_reporter/load_data_store.cc, line 70 at r2 (raw file):

PerHostStore::~PerHostStore() {
  for (auto it_per_balancer_store : per_balancer_stores_) {

Note that the type of it_per_balancer_store is actually std::pair<>, not an iterator type, so the it_ prefix isn't really appropriate. Also, the per_balancer_store part isn't really necessary, since it's obvious from the loop statement what we're looping over.

I generally just use p (short for "pair") as the name of the loop variable in this kind of situation. :)

Same thing throughout this PR.

src/cpp/server/load_reporter/load_reporter.h, line 35 at r2 (raw file):

#include "src/proto/grpc/lb/v1/load_reporter.grpc.pb.h"

using grpc::lb::v1::CallMetricData;

We should not have using statements in a .h file, as per:

https://google.github.io/styleguide/cppguide.html#Aliases

src/cpp/server/load_reporter/load_reporter.h, line 74 at r2 (raw file):

 public:
  // TODO(juanlishen): allow config for providers.
  // Takes ownership of the CensusViewProvider and CpuStatsProvider.

If it takes ownership, how about changing the parameters to be std::unique_ptr<>, so that the compiler enforces the ownership transfer?

src/cpp/server/load_reporter/load_reporter.h, line 106 at r2 (raw file):

 private:
  typedef struct LoadBalancingFeedbackRecord {

No need for the typedef here; in C++, struct names are automatically usable as bare types.

src/cpp/server/load_reporter/load_reporter.h, line 107 at r2 (raw file):

 private:
  typedef struct LoadBalancingFeedbackRecord {
    std::chrono::system_clock::time_point end_time_;

Struct members, unlike class members, should not have an underscore suffix.

https://google.github.io/styleguide/cppguide.html#Variable_Names

src/cpp/server/load_reporter/load_reporter.h, line 134 at r2 (raw file):

  std::unique_ptr<CpuStatsProvider> cpu_stats_provider_;
  std::deque<LoadBalancingFeedbackRecord> feedback_records_;
  // TODO(juanlishen): Lock in finer grain. Locking the whole store may be

Do we have any idea what kind of traffic we'll be seeing here? It might be useful to try to get an idea of how performant this actually needs to be, since that may influence the design.

src/cpp/server/load_reporter/load_reporter.cc, line 30 at r2 (raw file):

  // Read the accumulative CPU stats.
  FILE* fp;
  fp = fopen("/proc/stat", "r");

This will only work on Linux. What will we do on (e.g.) MacOS or Windows?

src/cpp/server/load_reporter/load_reporter_async_service_impl.h, line 31 at r2 (raw file):

#include "src/proto/grpc/lb/v1/load_reporter.grpc.pb.h"

constexpr uint32_t FEEDBACK_SAMPLE_WINDOW_SECONDS = 10;

Constant names should be of the form described here:

https://google.github.io/styleguide/cppguide.html#Constant_Names

src/cpp/server/load_reporter/load_reporter_async_service_impl.h, line 33 at r2 (raw file):

constexpr uint32_t FEEDBACK_SAMPLE_WINDOW_SECONDS = 10;
constexpr uint32_t FETCH_AND_SAMPLE_INTERVAL_SECONDS = 1;
// TODO(juanlishen): Update the version number with the PR number every time

How about using the gRPC version number instead?

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 25 at r2 (raw file):

LoadReporterAsyncServiceImpl::LoadReporterAsyncServiceImpl() {
  // TODO(juanlishen): Use make_unique.

Any reason not to just do this?

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 33 at r2 (raw file):

LoadReporterAsyncServiceImpl& LoadReporterAsyncServiceImpl::instance() {
  static LoadReporterAsyncServiceImpl instance_;

This is disallowed by the style guide:

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

A simple work-around is to make the static variable a pointer, set it to point to an instance allocated via new, and return the pointer.

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 67 at r2 (raw file):

void LoadReporterAsyncServiceImpl::Run() {
  // TODO(juanlishen): Which port to use?

This shouldn't be deciding on the port itself or encapsulating the ServerBuilder. Instead, we should set this up as a service that is automatically added to the ServerBuilder created by the application.

@yang-g may be able to help you figure out how to structure this. I know that we have a bunch of services that get automatically added internally (e.g., reflection, health-checking, etc), but I don't know if we're currently doing anything like that in OSS.

src/cpp/server/load_reporter/util.h, line 30 at r2 (raw file):

template <typename K, typename V>
bool UnorderedMultimapEraseKeyValue(std::unordered_multimap<K, V>& map,

For all of the functions in this file, it seems like it would be more efficient to replace std::unordered_multimap<K, V> with std::unordered_map<K, std::set<V>>.

src/cpp/server/load_reporter/util.h, line 43 at r2 (raw file):

template <typename K, typename V>
std::vector<K> UnorderedMultimapKeys(const std::unordered_multimap<K, V>& map) {

This seems fairly inefficient, although it may be fine if it's not used often. If this does get used a lot, and if you take my suggestion above about using std::unordered_map<> instead of std::unordered_multimap<>, then an alternative would be an iterator view of some sort. (Come chat with me if you want more info about what I mean by this.)

src/proto/grpc/lb/v1/load_reporter.proto, line 23 at r2 (raw file):

package grpc.lb.v1;

message Duration {

This looks like a duplicate of the well-known google.protobuf.Duration proto. Instead of defining that again here, please change this to depend on that proto.

Note that @ncteisen recently did something like this for the channelz proto, so he can probably help you iron out any build dependency issues.

src/proto/grpc/lb/v1/load_reporter.proto, line 96 at r2 (raw file):

  // Optional server_version should be a value that is modified (and
  // monotonically increased) when changes are made to the server
  // implementation. In google3 this will be the original CL# of the change.

The last sentence of this comment does not apply to OSS and should be removed.

test/cpp/server/load_reporter/load_data_store_test.cc, line 36 at r2 (raw file):

namespace {

const grpc::string HOSTNAME_1 = "HOSTNAME_1";

Same comment about const naming as I mentioned in one of the other files.

test/cpp/server/load_reporter/load_data_store_test.cc, line 36 at r3 (raw file):

namespace {

const grpc::string HOSTNAME_1 = "HOSTNAME_1";

We're not supposed to have global variables of non-POD type, as I mentioned elsewhere.

Comments from Reviewable

Thanks for reviewing!

@summerxyt @slash-lib, please take a look at the logic around load data. This PR is WIP regarding the Census part.

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks broke.

build.yaml, line 1880 at r3 (raw file):

Done.

src/cpp/server/load_reporter/load_data_store.h, line 31 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_data_store.h, line 50 at r2 (raw file):

Both done.

src/cpp/server/load_reporter/load_data_store.h, line 87 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_data_store.h, line 94 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_data_store.h, line 135 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_data_store.h, line 221 at r2 (raw file):

LoadDataStore needs to access the member variables to find some particular PerBalancerStore. And I want to avoid making per_balancer_stores_ and assigned_stores_ public.

Upon further thought, I should remove the friend class and add some public methods for the searching.

src/cpp/server/load_reporter/load_data_store.h, line 233 at r2 (raw file):

Done. std::shared_ptr<> is used for intended sharing between per_balancer_stores_ and assigned_stores_.

I've converted all the relevant usage about those two member variables to std::shared_ptr<>. Should I do so? Or only use smart pointers here?

src/cpp/server/load_reporter/load_data_store.cc, line 70 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 70 at r2 (raw file):

Done. Good catch!

I've shortened the name of some loop variables unless the loop body is long or complex.

src/cpp/server/load_reporter/load_reporter.h, line 35 at r2 (raw file):

Removed. Thanks for pointing this out; I didn't realize that before.

Also changed some declarations to use auto to save some typing.

src/cpp/server/load_reporter/load_reporter.h, line 74 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_reporter.h, line 106 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_reporter.h, line 107 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_reporter.h, line 134 at r2 (raw file):

@slash-lib @summerxyt may have more context about this.

src/cpp/server/load_reporter/load_reporter.cc, line 30 at r2 (raw file):

Done.

Sorry, I should've added a TODO here.

src/cpp/server/load_reporter/load_reporter_async_service_impl.h, line 31 at r2 (raw file):

Done.

src/cpp/server/load_reporter/load_reporter_async_service_impl.h, line 33 at r2 (raw file):

In the protobuf file, server_version should be modified when the server (@summerxyt, it should be server load reporter right?) implementation is changed. This is to be consistent with the internal implementation.

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 25 at r2 (raw file):

It can't compile with make_unique. I just noticed it's a C++14 feature. I have deleted the TODO.

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 33 at r2 (raw file):

Done.

So I also changedcq_, service_, and load_reporter_ in LoadReporterAsyncServiceImpl to non-static and pass them to each handler.

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 67 at r2 (raw file):

Looks like ServerBuilderOption should be used to start the service automatically. I will do that after the service is fully done.

src/cpp/server/load_reporter/util.h, line 30 at r2 (raw file):

Done.

Yes, the structure is clearer by using std::unordered_map<K, std::set<V>>, and there is some performance gain (but I guess this is not very significant because the internal data structure of std::unordered_multimap<K, V> also maps key to a bucket of values). The con is that insertion is a little more complex now.

I think a strong argument to use std::unordered_map<K, std::set<V>> is that duplicate key-value pairs are not expected in our case.

src/cpp/server/load_reporter/util.h, line 43 at r2 (raw file):

Done.

I guess iterator view means iterating all the keys and storing all of them?

src/proto/grpc/lb/v1/load_reporter.proto, line 23 at r2 (raw file):

Done.

Will update load_balancer.proto later.

src/proto/grpc/lb/v1/load_reporter.proto, line 96 at r2 (raw file):

Done. Sorry about that.

test/cpp/server/load_reporter/load_data_store_test.cc, line 36 at r2 (raw file):

Done.

test/cpp/server/load_reporter/load_data_store_test.cc, line 36 at r3 (raw file):

Done. Moved to test suite class. The style guide does specify how to name constants and class member variables, but doesn't specify how to name const class members. I keep the name style of constants because we originally want some constants, and we make them class member variables because of other restriction.

Comments from Reviewable

This is moving in the right direction. Please let me know if you have any questions.

Reviewed 2 of 5 files at r4, 10 of 10 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

src/cpp/server/load_reporter/load_data_store.h, line 233 at r2 (raw file):

Well, shared_ptr<> is a type of smart pointer. But I know what you mean. :)

The style guide discourages use of shared_ptr<> but does not disallow it:

https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers

There's even more detailed guidance about this in the internal style guide. In general, I think shared_ptr<> is only really a good idea in cases where you have two owners and you don't know which of them will give up ownership first.

Is there a reasonable way to avoid using shared_ptr<> here? For example, could we use unique_ptr<> in per_balancer_stores_ and have the pointer in assigned_stores_ just reference the object owned by per_balancer_stores_?

src/cpp/server/load_reporter/load_data_store.h, line 125 at r5 (raw file):

    latency_ms_ += other.latency_ms_;
    for (const auto& p : other.call_metrics_) {
      call_metrics_.insert({p.first, CallMetricValue()})

I think it would be easier to understand and slightly more efficient to say this:

call_metrics_[p.first].MergeFrom(p.second);

This approach avoids instantiating a CallMetricValue object that will just be thrown away if the entry is already in the map.

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):

  const grpc::string& lb_id() const { return lb_id_; }
  const grpc::string& load_key() const { return load_key_; }
  std::unordered_map<LoadRecordKey, LoadRecordValue, LoadRecordKey::Hasher>&

How about adding a typedef for this, so we don't have to repeat it twice?

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):

  const grpc::string& lb_id() const { return lb_id_; }
  const grpc::string& load_key() const { return load_key_; }
  std::unordered_map<LoadRecordKey, LoadRecordValue, LoadRecordKey::Hasher>&

Methods should not return non-const references. This should either be a const reference (in which case the method should also be const) or a non-const pointer.

src/cpp/server/load_reporter/load_data_store.cc, line 33 at r5 (raw file):

  // During suspension, the load data received will be dropped.
  if (!suspended_) {
    container_.insert({key, LoadRecordValue()}).first->second.MergeFrom(value);

container_[key].MergeFrom(value);

src/cpp/server/load_reporter/load_data_store.cc, line 101 at r5 (raw file):

  // The stores that were assigned to this balancer are orphaned now. They
  // should be re-assigned to other balancers which are still receiving reports.
  auto orphaned_stores = UnorderedMapOfSetExtract(assigned_stores_, lb_id);

Instead of using UnorderedMapOfSetExtract(), which creates a temporary copy of the value set, why not just iterate over assigned_stores_[lb_id], and then delete the key after the loop exits?

src/cpp/server/load_reporter/load_data_store.cc, line 103 at r5 (raw file):

  auto orphaned_stores = UnorderedMapOfSetExtract(assigned_stores_, lb_id);
  for (auto& orphaned_store : orphaned_stores) {
    auto lb_ids_with_same_load_key = UnorderedMapOfSetFindAll(

Using the RandomElement() template I describe below, I think the body of this loop can be written a bit more efficiently:

grpc::string* new_receiver = nullptr;
auto it = load_key_to_receiving_lb_ids_.find(orphaned_store->load_key());
if (it != load_key_to_receiving_lb_ids_.end()) {
  // First, try to pick from the active balancers with the same load key.
  auto& candidates = it->second;
  new_receiver = RandomElement(&candidates);
} else {
  // If failed, pick from all the active balancers.
  auto* p = RandomElement(&assigned_stores_);
  new_receiver = &p->first;
}
if (new_receiver != nullptr) {
  AssignOrphanedStore(orphaned_store, *new_receiver);
} else {
  // Load data for an LB ID that can't be assigned to any stream should
  // be dropped.
  orphaned_store->Suspend();
}

This avoids creating a temporary set to hold the candidates.

src/cpp/server/load_reporter/load_data_store.cc, line 118 at r5 (raw file):

      orphaned_store->Suspend();
    } else {
      auto new_receiver = std::vector<grpc::string>(

I think this can be done without instantiating an unnecessary vector by using std::advance(). Try something like this:

template <typename C>
typename C::value_type* RandomElement(C* container) {
  auto it = container->begin();
  std::advance(it, std::rand() % container->size());
  return &(*it);
}

src/cpp/server/load_reporter/load_data_store.cc, line 133 at r5 (raw file):

}

const std::set<std::shared_ptr<PerBalancerStore>>

Instead of constructing a new set to return, I suggest having this return a pointer to the original set, which can be null if lb_id is not found in the map.

src/cpp/server/load_reporter/load_data_store.cc, line 145 at r5 (raw file):

  auto it = assigned_stores_.find(new_receiver);
  GPR_ASSERT(it != assigned_stores_.end());
  it->second.insert(orphaned_store);

If we stick with shared_ptr<>, we probably want to use std::move() here, so that we aren't unnecessarily mutating the ref count by making a copy. Note that this will probably require moving this statement after the logging statement below, since it also accesses orphaned_store.

src/cpp/server/load_reporter/load_data_store.cc, line 159 at r5 (raw file):

  GPR_ASSERT(per_balancer_stores_.find(lb_id) == per_balancer_stores_.end());
  GPR_ASSERT(assigned_stores_.find(lb_id) == assigned_stores_.end());
  load_key_to_receiving_lb_ids_.insert({load_key, {}})

load_key_to_receiving_lb_ids_[load_key].insert(lb_id);

src/cpp/server/load_reporter/load_data_store.cc, line 164 at r5 (raw file):

      new PerBalancerStore(lb_id, load_key));
  per_balancer_stores_.insert({lb_id, per_balancer_store});
  assigned_stores_.insert({lb_id, {per_balancer_store}});

Use std::move() here to avoid unnecessarily mutating the ref count.

src/cpp/server/load_reporter/load_reporter.cc, line 30 at r5 (raw file):

  uint64_t busy = 0, total = 0;
  // Read the accumulative CPU stats.
#if defined(GPR_LINUX)

We generally don't want these kind of conditional macros littered all over the code. Instead, we should have a separate file implementing this interface for each platform, and the entire file should be wrapped in a conditional. In other words, we should have a load_reporter_linux.cc where the entire file is wrapped in a #ifdef GPR_LINUX block, and then a load_reporter_windows.cc where the entire file is wrapped in a #ifdef GPR_WINDOWS block, etc.

@nicolasnoble can give you more pointers on how to set this up in the build system.

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 40 at r5 (raw file):

LoadReporterAsyncServiceImpl* LoadReporterAsyncServiceImpl::GetInstance() {
  if (instance_ == nullptr) {

Do we have some guarantee that this will only be invoked from one thread at a time? If not, we'll need some synchronization.

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 47 at r5 (raw file):

void LoadReporterAsyncServiceImpl::ResetInstance() {
  if (instance_ != nullptr) {

Same here.

src/cpp/server/load_reporter/util.h, line 43 at r2 (raw file):

No. What I mean is a custom iterator class that wraps the original map but returns only the keys.

You can think of this as a way of getting the kind of iterator you would get from std::vector<K> for an underlying object of type std::unordered_map<K, std::set<V>>. The iterator would actually iterate over the elements of the map, just like the map's normal iterator, but *it would return K& instead of std::pair<K, std::set<V>>.

I think we have an internal implementation of this kind of thing called key_view, but it looks like it's not in absl, unfortunately. But it probably wouldn't be too hard to rig up something like this.

In any case, I think that if you take my suggestion for how to simplify the code in load_data_store.cc, then this function is no longer needed, so this issue becomes moot.

src/cpp/server/load_reporter/util.h, line 32 at r5 (raw file):

bool UnorderedMapOfSetEraseKeyValue(std::unordered_map<K, std::set<V>>& map,
                                    const K& key, const V& value) {
  return map.find(key) != map.end() ? map.find(key)->second.erase(value)

If we erase the last element in the value set, do we also want to remove the key from the map?

src/cpp/server/load_reporter/util.h, line 47 at r5 (raw file):

template <typename K, typename V>
std::set<V> UnorderedMapOfSetFindAll(

These two functions are also probably not needed if you take my suggestions in load_data_store.cc.

test/cpp/server/load_reporter/load_data_store_test.cc, line 36 at r5 (raw file):

namespace {

using namespace ::grpc::load_reporter;

The style guide prohibits a using statement that imports an entire namespace:

https://google.github.io/styleguide/cppguide.html#Namespaces

Instead, you'll need a using statement for each specific symbol you want to use from that namespace.

Comments from Reviewable

Review status: 3 of 11 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

src/cpp/server/load_reporter/load_data_store.cc, line 215 at r16 (raw file):

Done.

Comments from Reviewable

I will merge it when I get a LGTM from @summerxyt .

LGTM

• Basic Tests MacOS [opt] (internal CI): TIMEOUT: run_tests_grpc-node_macos_opt_native #15245
• Bazel Debug build for C/C++: Most of the Bazel targets fail to be built #15246
• Bazel Opt build for C/C++: Most of the Bazel targets fail to be built #15246
• Interop ALTS Tests (internal CI): FAILED: cloud_to_cloud:go:c++_server:unimplemented_method #15247
• Interop Cloud-to-Cloud Tests (internal CI): FAILED: cloud_to_cloud:go:c++_server:unimplemented_method #15247
• Interop Cloud-to-Prod Tests (internal CI): TIMEOUT: cloud_to_prod:python #15231