Release 2.4.0

New Module
From this version, more project module will join Apache Pegasus Project. In this version, the following projects are included:

RDSN: In the past, rdsn exists as a sub-project in current repository, in this release, it officially become the core module of the project.
PegasusClient: Pegasus support multi-language client. In the past, it existed different repository, in this release, the following clients are included in the repository:

Pegasus-Java-Client
Pegasus-Scala-Client
Pegasus-Golang-Client
Pegasus-Python-Client
Pegasus-NodeJS-Client

PegasusDocker: Pegasus support docker-build, now it is included the latest version
PegasusShell: In the past, Pegasus use c++ shell to manage the cluster, now in the latest version, we bring new shell tools: AdminCli and Pegic

New architecture
In this version, we remove the shared log to enhance the pegasus performance,  Related pull request as follow:

https://github.com/XiaoMi/rdsn/pull/993
https://github.com/XiaoMi/rdsn/pull/994
https://github.com/XiaoMi/rdsn/pull/999
https://github.com/XiaoMi/rdsn/pull/1019
https://github.com/XiaoMi/rdsn/pull/1022
https://github.com/XiaoMi/rdsn/pull/1048
https://github.com/XiaoMi/rdsn/pull/1028
https://github.com/apache/incubator-pegasus/pull/890

New Feature
Replica-factor update
Supporting flexible replica count. In the past, the replica factor was Immutable once one table was created. In current version, user can dynamically adjust the factor of specified table. Related pull request as follow:

https://github.com/XiaoMi/rdsn/pull/1061
https://github.com/XiaoMi/rdsn/pull/1072
https://github.com/XiaoMi/rdsn/pull/1077
https://github.com/XiaoMi/rdsn/pull/1087
https://github.com/XiaoMi/rdsn/pull/1109
https://github.com/XiaoMi/rdsn/pull/1110
https://github.com/XiaoMi/rdsn/pull/1115
https://github.com/apache/incubator-pegasus/pull/914
https://github.com/apache/incubator-pegasus/pull/999
https://github.com/apache/incubator-pegasus/pull/1035

Read Request Limiter
In the past, we only support write limiter, in this version,  we add the supporting for read:

https://github.com/XiaoMi/rdsn/pull/941
https://github.com/XiaoMi/rdsn/pull/939
https://github.com/apache/incubator-pegasus/pull/829
https://github.com/XiaoMi/rdsn/pull/948
https://github.com/XiaoMi/rdsn/pull/947
https://github.com/XiaoMi/rdsn/pull/946

Jemalloc Support

https://github.com/XiaoMi/rdsn/pull/910
https://github.com/apache/incubator-pegasus/pull/1050

Build Feature
We have made some restrictions on the compilation environment and support MacOS and aarcch64:

https://github.com/XiaoMi/rdsn/pull/1041
https://github.com/XiaoMi/rdsn/pull/1034
https://github.com/XiaoMi/rdsn/pull/1097
https://github.com/apache/incubator-pegasus/pull/1049

New BatchGetAPI
In the past, the batchGet implement based the singleGet, the latest version will aggregate different request first berfore sending, it will improve the performace:

https://github.com/apache/incubator-pegasus/pull/897
https://github.com/XiaoMi/pegasus-java-client/pull/175

Task Queue limiter

https://github.com/XiaoMi/rdsn/pull/902
https://github.com/apache/incubator-pegasus/pull/831

Feature enhancement
Bulkload
We improve bulkload feature to reduce the io-load of downloading and ingesting, besides, we offer better interfaces and failure handling logic, the related pull request as follow:

https://github.com/XiaoMi/rdsn/pull/952
https://github.com/XiaoMi/rdsn/pull/958
https://github.com/XiaoMi/rdsn/pull/959
https://github.com/XiaoMi/rdsn/pull/960
https://github.com/XiaoMi/rdsn/pull/962
https://github.com/XiaoMi/rdsn/pull/964
https://github.com/XiaoMi/rdsn/pull/967
https://github.com/XiaoMi/rdsn/pull/1004
https://github.com/XiaoMi/rdsn/pull/1011
https://github.com/XiaoMi/rdsn/pull/1018
https://github.com/XiaoMi/rdsn/pull/1027
https://github.com/XiaoMi/rdsn/pull/1031
https://github.com/XiaoMi/rdsn/pull/1035
https://github.com/XiaoMi/rdsn/pull/1039
https://github.com/XiaoMi/rdsn/pull/1102
https://github.com/XiaoMi/rdsn/pull/1103
https://github.com/XiaoMi/rdsn/pull/1104
https://github.com/XiaoMi/rdsn/pull/1105
https://github.com/XiaoMi/rdsn/pull/1069
https://github.com/XiaoMi/rdsn/pull/1074
https://github.com/XiaoMi/rdsn/pull/1009
https://github.com/apache/incubator-pegasus/pull/881
https://github.com/apache/incubator-pegasus/pull/888
https://github.com/apache/incubator-pegasus/pull/968
https://github.com/apache/incubator-pegasus/pull/975
https://github.com/XiaoMi/rdsn/pull/1002
https://github.com/apache/incubator-pegasus/pull/868

Duplication
In the past, duplication has some shortcoming:  It depends remote filesystem to sync the checkpoint; The synchronization of plog data only sends a single mutation at each RPC. In this version, we enhance the above problem(the detail design see https://github.com/apache/incubator-pegasus/issues/892), related pull request as follows:

https://github.com/XiaoMi/rdsn/pull/1038
https://github.com/XiaoMi/rdsn/pull/1040
https://github.com/XiaoMi/rdsn/pull/1045
https://github.com/XiaoMi/rdsn/pull/1046
https://github.com/XiaoMi/rdsn/pull/1049
https://github.com/XiaoMi/rdsn/pull/1051
https://github.com/XiaoMi/rdsn/pull/1053
https://github.com/XiaoMi/rdsn/pull/1055
https://github.com/XiaoMi/rdsn/pull/1056
https://github.com/XiaoMi/rdsn/pull/1059
https://github.com/XiaoMi/rdsn/pull/1060
https://github.com/XiaoMi/rdsn/pull/1063
https://github.com/XiaoMi/rdsn/pull/1064
https://github.com/XiaoMi/rdsn/pull/1065
https://github.com/apache/incubator-pegasus/pull/917
https://github.com/XiaoMi/rdsn/pull/1066
https://github.com/XiaoMi/rdsn/pull/1067
https://github.com/apache/incubator-pegasus/pull/919
https://github.com/XiaoMi/rdsn/pull/1071
https://github.com/XiaoMi/rdsn/pull/1076
https://github.com/XiaoMi/rdsn/pull/1080
https://github.com/apache/incubator-pegasus/pull/930
https://github.com/XiaoMi/rdsn/pull/1084
https://github.com/apache/incubator-pegasus/pull/935
https://github.com/apache/incubator-pegasus/pull/936
https://github.com/XiaoMi/rdsn/pull/1085
https://github.com/apache/incubator-pegasus/pull/940
https://github.com/XiaoMi/rdsn/pull/1121
https://github.com/apache/incubator-pegasus/pull/1007
https://github.com/apache/incubator-pegasus/pull/1008
https://github.com/XiaoMi/rdsn/pull/976
https://github.com/apache/incubator-pegasus/pull/1065
https://github.com/apache/incubator-pegasus/pull/1078

PerfCounter
In the version, we support new metric implement to optimize performance:

https://github.com/XiaoMi/rdsn/pull/1033
https://github.com/XiaoMi/rdsn/pull/1070
https://github.com/XiaoMi/rdsn/pull/1073
https://github.com/XiaoMi/rdsn/pull/1075
https://github.com/XiaoMi/rdsn/pull/1081
https://github.com/apache/incubator-pegasus/pull/1074

Manual Compaction

https://github.com/XiaoMi/rdsn/pull/989
https://github.com/XiaoMi/rdsn/pull/987
https://github.com/XiaoMi/rdsn/pull/983
https://github.com/apache/incubator-pegasus/pull/854
https://github.com/XiaoMi/rdsn/pull/981

Learn with NFS
To reduce the impact of data migration for IO-LOAD and ensure the migration rate, our data transmission supports disk level speed limits：

https://github.com/XiaoMi/rdsn/pull/944
https://github.com/XiaoMi/rdsn/pull/943
https://github.com/XiaoMi/rdsn/pull/985

Latency Tracer
The latest latency tracer support perf-counter and fix some bugs:

https://github.com/XiaoMi/rdsn/pull/1029
https://github.com/XiaoMi/rdsn/pull/1023
https://github.com/XiaoMi/rdsn/pull/951
https://github.com/XiaoMi/rdsn/pull/945
https://github.com/XiaoMi/rdsn/pull/965

Other important

https://github.com/XiaoMi/rdsn/pull/1086
https://github.com/XiaoMi/rdsn/pull/988
https://github.com/XiaoMi/rdsn/pull/979
https://github.com/XiaoMi/rdsn/pull/978
https://github.com/XiaoMi/rdsn/pull/963
https://github.com/apache/incubator-pegasus/pull/870
https://github.com/apache/incubator-pegasus/pull/852
https://github.com/apache/incubator-pegasus/pull/907
https://github.com/apache/incubator-pegasus/pull/1009
https://github.com/apache/incubator-pegasus/pull/1085
https://github.com/apache/incubator-pegasus/pull/1061

Java Client

https://github.com/XiaoMi/pegasus-java-client/pull/184
https://github.com/XiaoMi/pegasus-java-client/pull/181
https://github.com/XiaoMi/pegasus-java-client/pull/180
https://github.com/XiaoMi/pegasus-java-client/pull/177
https://github.com/apache/incubator-pegasus/pull/1019
https://github.com/apache/incubator-pegasus/pull/1000
https://github.com/apache/incubator-pegasus/pull/973

Go Client

https://github.com/XiaoMi/pegasus-go-client/pull/110
https://github.com/XiaoMi/pegasus-go-client/pull/109
https://github.com/XiaoMi/pegasus-go-client/pull/107
https://github.com/XiaoMi/pegasus-go-client/pull/105
https://github.com/XiaoMi/pegasus-go-client/pull/104
https://github.com/XiaoMi/pegasus-go-client/pull/102
https://github.com/XiaoMi/pegasus-go-client/pull/101
https://github.com/XiaoMi/pegasus-go-client/pull/99

Python Client

https://github.com/apache/incubator-pegasus/pull/977

Admin Cli

support nodes migrator

https://github.com/pegasus-kv/admin-cli/pull/62
https://github.com/pegasus-kv/admin-cli/pull/60
https://github.com/pegasus-kv/admin-cli/pull/59
https://github.com/pegasus-kv/admin-cli/pull/54
https://github.com/pegasus-kv/admin-cli/pull/53
https://github.com/pegasus-kv/admin-cli/pull/52
https://github.com/pegasus-kv/admin-cli/pull/51
https://github.com/pegasus-kv/admin-cli/pull/50

https://github.com/pegasus-kv/admin-cli/pull/58
https://github.com/pegasus-kv/admin-cli/pull/57
https://github.com/pegasus-kv/admin-cli/pull/56
https://github.com/pegasus-kv/admin-cli/pull/55
https://github.com/apache/incubator-pegasus/pull/987
https://github.com/apache/incubator-pegasus/pull/969
https://github.com/apache/incubator-pegasus/pull/958
https://github.com/apache/incubator-pegasus/pull/1006
https://github.com/apache/incubator-pegasus/pull/976

Pegasus Docker

https://github.com/apache/incubator-pegasus/pull/1011

Code Refactor

https://github.com/XiaoMi/rdsn/pull/1068
https://github.com/XiaoMi/rdsn/pull/1062
https://github.com/XiaoMi/rdsn/pull/1050
https://github.com/XiaoMi/rdsn/pull/1032
https://github.com/XiaoMi/rdsn/pull/1030
https://github.com/XiaoMi/rdsn/pull/1015
https://github.com/XiaoMi/rdsn/pull/1012
https://github.com/XiaoMi/rdsn/pull/1010
https://github.com/XiaoMi/rdsn/pull/1010
https://github.com/XiaoMi/rdsn/pull/1003
https://github.com/XiaoMi/rdsn/pull/1000
https://github.com/XiaoMi/rdsn/pull/998
https://github.com/XiaoMi/rdsn/pull/997
https://github.com/XiaoMi/rdsn/pull/996
https://github.com/XiaoMi/rdsn/pull/995
https://github.com/XiaoMi/rdsn/pull/992
https://github.com/XiaoMi/rdsn/pull/974
https://github.com/XiaoMi/rdsn/pull/968
https://github.com/XiaoMi/rdsn/pull/950
https://github.com/XiaoMi/rdsn/pull/942
https://github.com/apache/incubator-pegasus/pull/1021
https://github.com/apache/incubator-pegasus/pull/1005
https://github.com/apache/incubator-pegasus/pull/921
https://github.com/apache/incubator-pegasus/pull/916

Bug Fix
Core

https://github.com/XiaoMi/rdsn/pull/1016
https://github.com/XiaoMi/rdsn/pull/1008
https://github.com/XiaoMi/rdsn/pull/1099
https://github.com/XiaoMi/rdsn/pull/1017
https://github.com/XiaoMi/rdsn/pull/1052

Common

https://github.com/XiaoMi/rdsn/pull/1047
https://github.com/XiaoMi/rdsn/pull/1088
https://github.com/XiaoMi/rdsn/pull/1044
https://github.com/XiaoMi/rdsn/pull/1037
https://github.com/XiaoMi/rdsn/pull/1036
https://github.com/XiaoMi/rdsn/pull/1001
https://github.com/XiaoMi/rdsn/pull/990
https://github.com/XiaoMi/rdsn/pull/984
https://github.com/XiaoMi/rdsn/pull/982
https://github.com/XiaoMi/rdsn/pull/980
https://github.com/apache/incubator-pegasus/pull/828
https://github.com/apache/incubator-pegasus/pull/984
https://github.com/apache/incubator-pegasus/pull/909
https://github.com/apache/incubator-pegasus/pull/911
https://github.com/apache/incubator-pegasus/pull/833

Performance
In this benchmark, we use the new machine, for the result is more reasonable, we re-run the Pegasus Server 2.3：

Machine parameters:  DDR4 16G * 8  | Intel Silver4210*2 2.20Ghz/3.20Ghz | SSD 480G * 8 SATA
Cluster Server: 3 * MetaServerNode  5 * ReplicaServerNode
YCSB Client: 3 * ClientNode
Request Length: 1KB(set/get)

Pegasus Server 2.3

Case
client and thread
R:W
R-QPS
R-Avg
R-P99
W-QPS
W-Avg
W-P99

Write Only
3 clients * 15 threads
0:1
-
-
-
48805
919
2124

Read Only
3 clients * 50 threads
1:0
370068
402
988
-
-
-

Read Write
3 clients * 30 threads
1:1
50762
532
5859
50759
1233
4162

Read Write
3 clients * 15 threads
1:3
14471
443
3869
43425
884
1899

Read Write
3 clients * 15 threads
1:30
1583
473
3432
47551
928
2066

Read Write
3 clients * 30 threads
3:1
119093
406
3367
39693
1035
2581

Read Write
3 clients * 50 threads
30:1
322904
435
1034
10762
882
1392

Pegasus Server 2.4

Case
client and thread
R:W
R-QPS
R-Avg
R-P99
W-QPS
W-Avg
W-P99

Write Only
3 clients * 15 threads
0:1
-
-
-
56953
787
1786

Read Only
3 clients * 50 threads
1:0
360642
413
984
-
-
-

Read Write
3 clients * 30 threads
1:1
62572
464
5274
62561
985
3764

Read Write
3 clients * 15 threads
1:3
16844
372
3980
50527
762
1551

Read Write
3 clients * 15 threads
1:30
1861
381
3557
55816
790
1688

Read Write
3 clients * 30 threads
3:1
140484
351
3277
46822
856
2044

Read Write
3 clients * 50 threads
30:1
336106
419
1221
11203
763
1276

Config-Update
+ [pegasus.server]
+ rocksdb_max_log_file_size = 8388608
+ rocksdb_log_file_time_to_roll = 86400
+ rocksdb_keep_log_file_num = 32

+ [replication]
+ plog_force_flush = false

- mutation_2pc_min_replica_count = 2
+ mutation_2pc_min_replica_count = 0 # 0 means it's value based table max replica count

+ enable_direct_io = false # Whether to enable direct I/O when download files from hdfs, default false
+ direct_io_buffer_pages = 64 # Number of pages we need to set to direct io buffer, default 64 which is recommend in my test.
+ max_concurrent_manual_emergency_checkpointing_count = 10

+ enable_latency_tracer_report = false
+ latency_tracer_counter_name_prefix = trace_latency

+ hdfs_read_limit_rate_mb_per_sec = 200
+ hdfs_write_limit_rate_mb_per_sec = 200

+ duplicate_log_batch_bytes = 0 # 0 means no batch before sending

+ [nfs]
- max_copy_rate_megabytes = 500
+ max_copy_rate_megabytes_per_disk = 0
- max_send_rate_megabytes = 500
+ max_send_rate_megabytes_per_disk = 0

+ [meta_server]
+ max_reserved_dropped_replicas = 0
+ bulk_load_verify_before_ingest = false
+ bulk_load_node_max_ingesting_count = 4
+ bulk_load_node_min_disk_count = 1
+ enable_concurrent_bulk_load = false
+ max_allowed_replica_count = 5
+ min_allowed_replica_count = 1

+ [task.LPC_WRITE_REPLICATION_LOG_SHARED]
+ enable_trace = true # true will mark the task will be traced latency if open global trace

Contributors
acelyc111
cauchy1988
empiredan
foreverneverer
GehaFearless
GiantKing
happydongyaoyao
hycdong
levy5307
lidingshengHHU
neverchanje
padmejin
Smityz
totalo
WHBANG
xxmazha
ZhongChaoqiang

Several problems have been found according to the checklist for Incubator release:

In LICENSE and .licenserc.yaml, some file paths have been not updated after refactor;
NOTICE year is still the last year.

Thus some PRs have been committed to fix these problems as follows:

[ ] https://github.com/apache/incubator-pegasus/pull/1123
[ ] https://github.com/apache/incubator-pegasus/pull/1121
[ ] https://github.com/apache/incubator-pegasus/pull/1119

These PRs should be cherry-picked to v2.4 to meet the requirements for Incubator release.

Since we have cherry-pick more commit into v2.4, which  involved the cmake module. Suggested by @acelyc111, I re-run benchmark as follow:
Pegasus Server 2.4

Case
client and thread
R:W
R-QPS
R-Avg
R-P99
W-QPS
W-Avg
W-P99

Write Only
3 clients * 15 threads
0:1
-
-
-
55490
808
3540

Read Only
3 clients * 50 threads
1:0
361112
414
997
-
-
-

Read Write
3 clients * 30 threads
1:1
63581
469
5447
63580
939
4959

Read Write
3 clients * 15 threads
1:3
16559
396
4228
49664
769
3987

Read Write
3 clients * 15 threads
1:30
1730
413
3669
51966
849
4735

Read Write
3 clients * 30 threads
3:1
135091
376
3007
45304
842
4753

Read Write
3 clients * 50 threads
30:1
319519
444
1442
10643
819
2691

Some more license issues have been resolved:

https://github.com/apache/incubator-pegasus/pull/1173
https://github.com/apache/incubator-pegasus/pull/1176

https://github.com/apache/incubator-pegasus/releases/tag/v2.4.0


Server-side load reporting service

Adding a service to report load to balancers. Please check src/proto/grpc/lb/v1/load_reporter.proto for details.
Eventually this service will be a server plugin, but that will be addressed in a future PR.
For the service itself, we still need some API support from https://github.com/census-instrumentation/opencensus-cpp to finish the stats write/read.
The classes are organized as following:

The visibility of some class member variables can be stricter, but the friendship will be a little verbose?
The classes can be declared in hierarchy, but they will be nested deeply.

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[trickle] No significant performance differences

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[microbenchmarks] No significant performance differences

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

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

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

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):
Previously, AspirinSJL (Juanli Shen) wrote…
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?

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):
Previously, AspirinSJL (Juanli Shen) wrote…
Done.
I guess iterator view means iterating all the keys and storing all of them?

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

[trickle] No significant performance differences

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[microbenchmarks] No significant performance differences

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

Upon code reviewer's request, this PR will be broken into smaller ones to make code review easier.
To keep some history of the discussion around the data storage and operations, I will keep the changes of LoadDataStore and its nested classes here. And I will move the load_reporter and LoadReporterAsyncServiceImpl into other PRs.

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

src/cpp/server/load_reporter/load_data_store.h, line 233 at r2 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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_?

Done.
Yes, unique_ptr<> is enough here. per_balancer_stores_ and assigned_stores_ should be deleted at the same time.

src/cpp/server/load_reporter/load_data_store.h, line 125 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

Done.

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
How about adding a typedef for this, so we don't have to repeat it twice?

Done.
(Using using because the style guide says "In new code, using is preferable to typedef, because it provides a more consistent syntax with the rest of C++ and works with templates.")

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

Done. But any reason not to use const reference here?

src/cpp/server/load_reporter/load_data_store.cc, line 33 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
container_[key].MergeFrom(value);

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 101 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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?

When we used multimap to store the assignment info, there was some problem if we change the content of the map while iterating through it. But now that we use a map of sets, there is better isolation so it's safe to change other sets' values while iterating through the orphaned set.
But another problem is that we need to skip the lb_id that is being disconnected. In other words, the candidate list is different from the key set of the current assigned_stores_ before we delete the closing lb_id. We need to do some additional condition check when we choose the new receiver randomly. And we probably need to add a while loop to find a valid new receiver if we want to avoid creating a new copy of candidate list.
So I think it's better to keep UnorderedMapOfSetExtract(), which makes the logic easy.

src/cpp/server/load_reporter/load_data_store.cc, line 103 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

Done.
Modified a little bit.

src/cpp/server/load_reporter/load_data_store.cc, line 118 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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);
}

Done.
That's a good alternative to random accessing by index.

src/cpp/server/load_reporter/load_data_store.cc, line 133 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 145 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

I see.
This has been changed to raw pointer.

src/cpp/server/load_reporter/load_data_store.cc, line 159 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
load_key_to_receiving_lb_ids_[load_key].insert(lb_id);

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 164 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Use std::move() here to avoid unnecessarily mutating the ref count.

I see.
Changed to raw pointer.

src/cpp/server/load_reporter/load_reporter.cc, line 30 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

Done.
I borrowed the idea in the ALTS code. https://github.com/grpc/grpc/blob/master/src/core/lib/security/credentials/alts/check_gcp_environment.h

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 40 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Do we have some guarantee that this will only be invoked from one thread at a time?  If not, we'll need some synchronization.

Done.
It's used in different stream handlers. But so far I think the service part itself is still single thread. Made instance_ atomic to be safe.

src/cpp/server/load_reporter/load_reporter_async_service_impl.cc, line 47 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Same here.

Done.

src/cpp/server/load_reporter/util.h, line 43 at r2 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

I see.
This function is deleted.

src/cpp/server/load_reporter/util.h, line 32 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
If we erase the last element in the value set, do we also want to remove the key from the map?

Done.
That's a good question. We should remove that key.

src/cpp/server/load_reporter/util.h, line 47 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
These two functions are also probably not needed if you take my suggestions in load_data_store.cc.

UnorderedMapOfSetFindAll() is deleted.
UnorderedMapOfSetExtract() is kept because of the reason mentioned in load_data_store.cc.

test/cpp/server/load_reporter/load_data_store_test.cc, line 36 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
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.

Done.

Comments from Reviewable

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

Hi @slash-lib @summerxyt , this PR is fully ready for review now. The PR still looks big, but that's because we have a lot of auto-generated code. Please take a look and let me know if there is any question. Thanks!

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[microbenchmarks] No significant performance differences

This is looking better.  Please let me know if you have any questions.  Thanks!

Reviewed 2 of 18 files at r10, 11 of 21 files at r11, 2 of 2 files at r13.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
Done. But any reason not to use const reference here?

No, I think using a const reference makes the most sense for accessor methods.  The only reason to do otherwise would be if the caller needed to modify the data, but that would really be a boundary violation anyway and would probably be better accomplished by providing other methods to do that modification.

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

class PerBalancerStore {
 public:
  using LoadRecords =

Suggest calling this LoadRecordMap.

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

  // TODO(juanlishen): Use bytestring protobuf type?
  grpc::string load_key_;
  LoadRecords container_;

Suggest calling this load_record_map_.

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

  PerBalancerStore* FindPerBalancerStore(const grpc::string& lb_id) const;

  const std::set<PerBalancerStore*>* GetAssignedStores(

Please document that this will return null if lb_id is not found.

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

      per_balancer_stores_;

  // Key: LB ID. The key set includes the LB IDs of the balancers that are

Please document that the values are pointers to the values stored in per_balancer_stores_.

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

                                         const grpc::string& lb_id) const;

  const std::set<PerBalancerStore*>* GetAssignedStores(const string& hostname,

Please document that this will return null if lb_id is not found.

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

  if (assigned_stores_.size() == 1) {
    for (const auto& p : per_balancer_stores_) {
      if (p.first != lb_id) {

Note the style guide recommendation on using local aliases for .first and .second in range-based for loops:
https://google.github.io/styleguide/cppguide.html#auto
Please audit all uses of .first and .second inside of range-based for loops in this PR and consider changing them to use this more readable form.

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

    for (const auto& p : per_balancer_stores_) {
      if (p.first != lb_id) {
        auto orphaned_store = p.second.get();

Please use PerBalancerStore* instead of auto.
General comment: auto should only be used in place of a very long type name (such as iterator types), where shortening the name aids readability.  It should not be used in place of an already-short name, since it actually hurts readability in that case by making the reader figure out what type is actually being returned.  For more guidance, see the style guide:
https://google.github.io/styleguide/cppguide.html#auto
I've called out a few specific places where I've noticed auto being used where it shouldn't be, but please audit all uses of auto and first and second in this PR to ensure that it is being used in a way that maximizes readability.

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

template <typename C>
static const typename C::value_type* RandomElement(const C& container) {

Instead of declaring this static, please put it in an anonymous namespace.

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

      load_key_to_receiving_lb_ids_, it_store_for_gone_lb->second->load_key(),
      lb_id));
  auto orphaned_stores = UnorderedMapOfSetExtract(assigned_stores_, lb_id);

Please use std::set<PerBalancerStore*> instead of auto.

src/cpp/server/load_reporter/load_data_store.cc, line 111 at r13 (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.
  for (auto orphaned_store : orphaned_stores) {

Suggest changing auto to PerBalancerStore*.

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

  GPR_ASSERT(assigned_stores_.find(lb_id) == assigned_stores_.end());
  load_key_to_receiving_lb_ids_[load_key].insert(lb_id);
  auto it = per_balancer_stores_

Suggest writing this in a more readable way:
std::unique_ptr<PerBalancerStore> per_balancer_store(
    new PerBalancerStore(lb_id, load_key));
assigned_stores_[lb_id] = per_balancer_store.get();
per_balancer_stores_[lb_id] = std::move(per_balancer_store);

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

                                        const grpc::string& lb_id,
                                        const grpc::string& load_key) {
  auto rc = per_host_stores_.insert(std::make_pair(hostname, PerHostStore()));

Suggest writing this in a more readable way:
per_host_stores_[hostname].ReportStreamCreated(lb_id, load_key);

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

template <typename K, typename V>
bool UnorderedMapOfSetEraseKeyValue(std::unordered_map<K, std::set<V>>& map,

If these utility functions are not going to be used anywhere except load_data_store.cc, then I suggest moving them there.

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

  auto it = map.find(key);
  if (it != map.end()) {
    auto erased = it->second.erase(value);

Let's use size_t instead of auto here,  so it's clearer to the reader what this value is.

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

template <typename K, typename V>
std::set<V> UnorderedMapOfSetExtract(std::unordered_map<K, std::set<V>>& map,

If we have to keep this, let's at least change it to avoid making unnecessary copies of the set.  Try changing it to return an rvalue reference and use std::move() within the function, both from the result of map.find(key)->second and in the return statement.

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

  if (it != map.end()) {
    auto set = map.find(key)->second;
    map.erase(key);

Since we already have the iterator pointing to the right entry, I think it would be more efficient to say map.erase(it).

Comments from Reviewable

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

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
No, I think using a const reference makes the most sense for accessor methods.  The only reason to do otherwise would be if the caller needed to modify the data, but that would really be a boundary violation anyway and would probably be better accomplished by providing other methods to do that modification.

So we should avoid direct data modification. I agree with that. So I guess the "non-const pointer" mentioned earlier means something like const LoadRecordMap *, right? The pointer is non-const; the data pointed to is const.

src/cpp/server/load_reporter/load_data_store.h, line 172 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Suggest calling this LoadRecordMap.

Done.

src/cpp/server/load_reporter/load_data_store.h, line 210 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Suggest calling this load_record_map_.

Done.

src/cpp/server/load_reporter/load_data_store.h, line 235 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please document that this will return null if lb_id is not found.

Done.

src/cpp/server/load_reporter/load_data_store.h, line 254 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please document that the values are pointers to the values stored in per_balancer_stores_.

Done.

src/cpp/server/load_reporter/load_data_store.h, line 274 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please document that this will return null if lb_id is not found.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 79 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Note the style guide recommendation on using local aliases for .first and .second in range-based for loops:
https://google.github.io/styleguide/cppguide.html#auto
Please audit all uses of .first and .second inside of range-based for loops in this PR and consider changing them to use this more readable form.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 80 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please use PerBalancerStore* instead of auto.
General comment: auto should only be used in place of a very long type name (such as iterator types), where shortening the name aids readability.  It should not be used in place of an already-short name, since it actually hurts readability in that case by making the reader figure out what type is actually being returned.  For more guidance, see the style guide:
https://google.github.io/styleguide/cppguide.html#auto
I've called out a few specific places where I've noticed auto being used where it shouldn't be, but please audit all uses of auto and first and second in this PR to ensure that it is being used in a way that maximizes readability.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 94 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Instead of declaring this static, please put it in an anonymous namespace.

Done.
Is there any reason to prefer anonymous namespace? I checked https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables, but didn't find the preference.

src/cpp/server/load_reporter/load_data_store.cc, line 108 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please use std::set<PerBalancerStore*> instead of auto.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 111 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Suggest changing auto to PerBalancerStore*.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 164 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Suggest writing this in a more readable way:
std::unique_ptr<PerBalancerStore> per_balancer_store(
    new PerBalancerStore(lb_id, load_key));
assigned_stores_[lb_id] = per_balancer_store.get();
per_balancer_stores_[lb_id] = std::move(per_balancer_store);

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 219 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Suggest writing this in a more readable way:
per_host_stores_[hostname].ReportStreamCreated(lb_id, load_key);

Done. So true.. I expanded the operator[]..

src/cpp/server/load_reporter/util.h, line 33 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
If these utility functions are not going to be used anywhere except load_data_store.cc, then I suggest moving them there.

Done.

src/cpp/server/load_reporter/util.h, line 37 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Let's use size_t instead of auto here,  so it's clearer to the reader what this value is.

Done.

src/cpp/server/load_reporter/util.h, line 47 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
If we have to keep this, let's at least change it to avoid making unnecessary copies of the set.  Try changing it to return an rvalue reference and use std::move() within the function, both from the result of map.find(key)->second and in the return statement.

Done.
(The return value is not changed so that Return Value Optimization is kept.)

src/cpp/server/load_reporter/util.h, line 52 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Since we already have the iterator pointing to the right entry, I think it would be more efficient to say map.erase(it).

Done.

Comments from Reviewable

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[microbenchmarks] No significant performance differences

This is looking pretty good.  All my remaining comments are minor.

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

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
So we should avoid direct data modification. I agree with that. So I guess the "non-const pointer" mentioned earlier means something like const LoadRecordMap *, right? The pointer is non-const; the data pointed to is const.

No, what I meant by "non-const pointer" is LoadRecordMap* (no const).  What I was trying to say is that any return value should generally be either const type& or type*, depending on whether or not you want the caller to be able to modify the value.  In general, we don't want the caller to be able to modify the value, so the first form is usually the right choice (especially for simple accessor methods), but I have sometimes run into unusual cases where mutability was the right thing.

src/cpp/server/load_reporter/load_data_store.cc, line 79 at r13 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
Done.

Please also add an alias for p.second.

src/cpp/server/load_reporter/load_data_store.cc, line 94 at r13 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
Done.
Is there any reason to prefer anonymous namespace? I checked https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables, but didn't find the preference.

Anonymous namespaces are just more idomatic in C++.

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

template <typename K, typename V>
bool UnorderedMapOfSetEraseKeyValue(std::unordered_map<K, std::set<V>>& map,

Please document these functions.

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

    size_t erased = it->second.erase(value);
    if (it->second.size() == 0) {
      map.erase(key);

Since we already have the iterator pointing at the right place here, I think it would be more efficient to say map.erase(it).

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

    auto set = std::move(map.find(key)->second);
    map.erase(it);
    return set;

Please use godbolt to verify that this will not actually construct a temporary set.

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

namespace load_reporter {

const grpc::string kInvalidLbId = "<INVALID_LBID_238dsb234890rb>";

Since these constants are the only things left here, I suggest moving them to load_data_store.h.
Also, consider making them constexpr.

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

namespace load_reporter {

const grpc::string kInvalidLbId = "<INVALID_LBID_238dsb234890rb>";

The style guide prohibits global variables with non-trivial destructors, which includes grpc::string.
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
Suggest making this a char array instead.

Comments from Reviewable

Thanks for the quick review!

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

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
No, what I meant by "non-const pointer" is LoadRecordMap* (no const).  What I was trying to say is that any return value should generally be either const type& or type*, depending on whether or not you want the caller to be able to modify the value.  In general, we don't want the caller to be able to modify the value, so the first form is usually the right choice (especially for simple accessor methods), but I have sometimes run into unusual cases where mutability was the right thing.

Thanks for explaining! I can understand that we need to add const when we don't want the caller to modify the data. But is there any reason that we don't use type& to offer mutability or use const type* for simple accessor methods? Is it just a convention to use const type& and type* ?

src/cpp/server/load_reporter/load_data_store.cc, line 79 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please also add an alias for p.second.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 94 at r13 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Anonymous namespaces are just more idomatic in C++.

I see.

src/cpp/server/load_reporter/load_data_store.cc, line 33 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please document these functions.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 39 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Since we already have the iterator pointing at the right place here, I think it would be more efficient to say map.erase(it).

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 53 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please use godbolt to verify that this will not actually construct a temporary set.

The original function doesn't generate assembly output in godbolt.  And the assembly for other lines seem unchanged. So I think returning directly should be OK.

src/cpp/server/load_reporter/util.h, line 25 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Since these constants are the only things left here, I suggest moving them to load_data_store.h.
Also, consider making them constexpr.

Done.

src/cpp/server/load_reporter/util.h, line 25 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
The style guide prohibits global variables with non-trivial destructors, which includes grpc::string.
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
Suggest making this a char array instead.

Done.

Comments from Reviewable

@summerxyt Gentle ping.

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

Just a couple of nits remaining.

Reviewed 6 of 6 files at r15.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
Thanks for explaining! I can understand that we need to add const when we don't want the caller to modify the data. But is there any reason that we don't use type& to offer mutability or use const type* for simple accessor methods? Is it just a convention to use const type& and type* ?

There's a strong convention at Google that we use references for const data and pointers for mutable data.  The style guide documents this for function parameters, not for return types, but the same convention applies to return types:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

src/cpp/server/load_reporter/load_data_store.cc, line 53 at r14 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
The original function doesn't generate assembly output in godbolt.  And the assembly for other lines seem unchanged. So I think returning directly should be OK.

The original function is a template, and templates don't compile unless they are instantiated somewhere.  Try putting the template into godbolt and then make a call to it.
I'm not sure that the simplified function you used will result in the same code, because it's a much simpler case.

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

      const grpc::string& other_lb_id = p.first;
      if (other_lb_id != lb_id) {
        const std::unique_ptr<PerBalancerStore>& orphaned_store = p.second;

Please move this outside of the if block.

Comments from Reviewable

Thanks for the review! @summerxyt @markdroth

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

src/cpp/server/load_reporter/load_data_store.h, line 200 at r5 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
There's a strong convention at Google that we use references for const data and pointers for mutable data.  The style guide documents this for function parameters, not for return types, but the same convention applies to return types:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Got it. Thanks!

src/cpp/server/load_reporter/load_data_store.h, line 60 at r15 (raw file):
Previously, summerxyt (Yetian Xia) wrote…
Need description on these members.

Done.
Renamed according to the proto.

src/cpp/server/load_reporter/load_data_store.h, line 98 at r15 (raw file):
Previously, summerxyt (Yetian Xia) wrote…
Does this particular hash function provide good hashing property, low conflict rate?

Hmm, good question, I'm not sure. I changed it so that it has the same effect with boost::hash_combine https://www.boost.org/doc/libs/1_55_0/doc/html/hash/reference.html#boost.hash_combine

src/cpp/server/load_reporter/load_data_store.h, line 175 at r15 (raw file):
Previously, summerxyt (Yetian Xia) wrote…
Please add comment on interface functions.

Done. Added comment to several functions.
Some functions still don't have comment because they are trivial or easy to understand from the interface itself. Please let me know if any one is unclear.

src/cpp/server/load_reporter/load_data_store.h, line 247 at r15 (raw file):
Previously, summerxyt (Yetian Xia) wrote…
Please rename this function. It is hard to infer its functionality from its name.

Done.

src/cpp/server/load_reporter/load_data_store.h, line 279 at r15 (raw file):
Previously, summerxyt (Yetian Xia) wrote…
Need comment on thread safety.
It seems all your operations need acquire a lock.

Done.
The synchronization is managed in load reporter level, which will be in a later PR. In the load reporter, we lock the whole load data store for mutual exclusion. This can be improved, but will be addressed in future PR.

src/cpp/server/load_reporter/load_data_store.h, line 285 at r15 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Making an unnecessary copy of the set is inefficient.  Returning a pointer to the underlying set avoids this.
The reason this returns a pointer instead of a reference is so that it can indicate that the hostname was not found by returning null.

Done. Added comment to clarify.

src/cpp/server/load_reporter/load_data_store.cc, line 53 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
The original function is a template, and templates don't compile unless they are instantiated somewhere.  Try putting the template into godbolt and then make a call to it.
I'm not sure that the simplified function you used will result in the same code, because it's a much simpler case.

Yes, I see the assembly after calling that function.
After changing to use std::move(), the output changes from
  lea rdx, [rbp-80]
  mov rax, QWORD PTR [rbp-88]
  mov rsi, rdx
  mov rdi, rax
  call std::set<int, std::less<int>, std::allocator<int> >::set(std::set<int, std::less<int>, std::allocator<int> >&&)

to
  lea rax, [rbp-80]
  mov rdi, rax
  call std::remove_reference<std::set<int, std::less<int>, std::allocator<int> >&>::type&& std::move<std::set<int, std::less<int>, std::allocator<int> >&>(std::set<int, std::less<int>, std::allocator<int> >&)
  mov rdx, rax
  mov rax, QWORD PTR [rbp-88]
  mov rsi, rdx
  mov rdi, rax
  call std::set<int, std::less<int>, std::allocator<int> >::set(std::set<int, std::less<int>, std::allocator<int> >&&)

Looks like the rvalue reference is added then removed.

src/cpp/server/load_reporter/load_data_store.cc, line 32 at r15 (raw file):
Previously, summerxyt (Yetian Xia) wrote…
If these helper functions are only used once, they only add difficulties to read and understand code, especially define them as template functions.

This is to separate the business logic and the operation on the data structure.
The business logic is already complex, putting the data structure operation here removes unnecessary complexity and distraction from the business logic. Also, if this helper function is not added, the variable names are long and cumbersome.
When separated into a helper function, the operation logic is also easier to understand because we only focus on this simple data structure.
I feel this level of template use is OK and actually beneficial. Otherwise, the function signature will be very long and complex.

src/cpp/server/load_reporter/load_data_store.cc, line 128 at r15 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Please move this outside of the if block.

Done.

src/cpp/server/load_reporter/load_data_store.cc, line 213 at r15 (raw file):
Previously, summerxyt (Yetian Xia) wrote…
Please avoid to call find() twice.

Done.

Comments from Reviewable

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

Just one minor nit.  Otherwise, this looks great to me.

Reviewed 3 of 3 files at r16.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

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

  auto it = per_host_stores_.find(hostname);
  if (it != per_host_stores_.end()) {
    return it->second.FindPerBalancerStore(lb_id);

This is another case where a local alias may help readability:
const PerHostStore& per_host_store = it->second;
return per_host_store.FindPerBalancerStore(lb_id);

Comments from Reviewable

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

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):
Previously, markdroth (Mark D. Roth) wrote…
This is another case where a local alias may help readability:
const PerHostStore& per_host_store = it->second;
return per_host_store.FindPerBalancerStore(lb_id);

Done.

Comments from Reviewable

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

Basic Tests MacOS [opt] (internal CI): #15245
Bazel Debug build for C/C++: #15246
Bazel Opt build for C/C++: #15246
Interop ALTS Tests (internal CI): #15247
Interop Cloud-to-Cloud Tests (internal CI): #15247
Interop Cloud-to-Prod Tests (internal CI): #15231


defining "after" slot in .show() method.

I'm attempting to append a div with a text class to the loader icon using the "after" slot in the .show() method. I've attempted to use jquery to create the markup, but it seems that I can only append text. I've event tried to query and target the vid-icon class to append.
Documentation is sparse when attempting to do this from the javascript side versus markup. Could someone assist?
...
        slotDefinition: {           after: $('.vid-icon').append('
loading ...
')         }
...
loader = Vue.$loading.show(propertiesDefinition, args.slotDefinition ? args.slotDefinition : {});

Please fill out the issue template

Issue Template?

But you are not new to internet
https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/configuring-issue-templates-for-your-repository
Just google it

I did some searching, and following this:
https://docs.github.com/en/github/managing-your-work-on-github/creating-an-issue
they said that if there are templates, then I would be presented with multiple options (step 4.)
I assure you, it takes me directly to a text area to insert the question, sans template options.

Okay, my mistake
Please specify this package version

We're using 3.4.2. I think I mainly am just looking for an answer to what type of object the "after" slot is looking for in the .show() method. It won't take a jquery object, Do I need to create a specific element via string conversion like jquery's ".parseHTML()"?

https://github.com/ankurk91/vue-loading-overlay/blob/19870761d9dbe38cffb24e352f1c4fee1b5fafa9/examples/components/plugin.vue#L93
Here is the example


Logic and performance

Improved logic by replacing if x>0 to x.
Removed math library as only 1 function was called from i.e log10 which saved memory.

For readability, I prefer the import and the conditionals as they are right now.

Ok great.


Ignore server error after input has changed

After the server returned validation error, is there a way to ignore it after input was changed?
Because we no longer know that input is invalid.

As I understand it when an error from the server occurs you want that error message to be ignored when the input is changed? The error should be reset every time setValue() is called. Does that not happen?

No, it does't happen. I invalidate input in handle submit function and check for error message in my input component with 'this.showError()'. I also do call this.setValue(val) when value changes. Can anyone confirm the issue?

hi again @frost555,
I will check this ASAP

Hi @frost555 ,
Very sorry about the delay here. I can confirm the error and will try to make a fix right now. Thanks for reporting!

Okay, fixed, there was indeed a bug here. Thanks for reporting @frost555 !


after a few logins i get error msgs

After a few logins i get (The game client is unable to gain access to the log-in server at this time. This is most commonly caused by firewall or router settings, security applications, or connecting through a campus network)    or it just says   [[ undefined ]]   error msgs and doesnt log me into my accounts and i  try to put the username  ect in and it still gives the error msgs it works until i log into the like 6th account+ and stops me from login in completely till it randomly goes away   any fixes?

I got same problem here, I can log in only two accounts in same time and other accounts drop to same error. I tried many things and nothing worked for me.

It's semi related to GW2 Launchbuddy in combination with 2FA, I think. Whenever my network changes GW2 launchbuddy manages to trigger 2 FA requests which result in 2 SMS that I receive. After a few accounts the rest of them get locked out. I asked A.Net support about it and they told me, they've locked access to the accounts (for 24hrs) because of too many password request attempts ( I assume FA requests count as well). Since I have 14 alt accounts running all on the same 2FA number, I trigger it pretty fast.
It's annoying but I'm not sure if fixing the double request would solve the issue completely.

I got the same issue, as a person who have 40+alts account i can"t log them in the same hour.
The first 20 accounts doesnt get any issue, but afterwards i get the same error message, so i need to wait 3 or 4 hours then i can open the rest without problem.
it's a pain and would be good to be fixed =)

https://github.com/TheCheatsrichter/Gw2_Launchbuddy/issues/268
Gonna close this issue and merge with newer issue.


eva: Add version 0.3.1

config location: $env:LocalAppData/NerdyPepper
$env:AppData/NerdyPepper
Closes #10515

[c] I have read the Contributing Guide.

/verify

CLI apps are usually expected to be maintained in Main bucket.

Please add this manifest to the main bucket instead of extras bucket


Not able to bring up the board based on STM32F103VET6 after port Alios-Things.

Describe the bug
failure to boot up board based on STM32F103VET6
environment

branch(version):   rel_1.3.2
Desktop OS(windows linux or macOS): Ubuntu 16.04
aos-cube :  0.2.46
make version: GNU Make 4.1
Python version : 2.7.12

Expected behavior
Boot up successully for helloworld
Screenshots
Additional context

MCU: STM32F103VET6

Already refer to the following manual for porting and give the changes accordingly for Cortex-M3
https://github.com/alibaba/AliOS-Things/wiki/Flash-and-KV-Storage-Porting-Guide

Also fork Alios-Things and create a new branch for porting. please refer to the following link and branch.
https://github.com/mullife/AliOS-Things/tree/rel_1.3.2_templedisplayer
the new board name is "displayer".  all project is copied from b_l475e and make the changes accordingly for porting.

build successfully with "aos make mqttapp@displayer " in Ubuntu 16.04 and get the bin/hex.  Flash teh binary by STM32CubeProgrammer. But look like that failure to jump to entry_main and also no any log come out by printf

Suppose may caused by toolchain, so switch to VS code in windows. but still failure to boo up. But check the same project with keil compiled with ARMCC and can boot up the board.

So for gcc compilation, any dependency on for STM32F103 porting and cause boot-up failure?
Already debug with a few days and but still failure. so hope that can get the support from here.
Thanks

Nobody can share any information for checking in further?

Thanks for reporting! @mullife
Would you please provide the boot up log?

Unfortunatly there is no any log printed. if any log printed, and then i can check in further by myself.
look like main() functions is not launched so all of initlization is not done.

try this fork，https://gitee.com/walker2048/AliOS-Things/tree/F103RB/
UART and GPIO part is good to go。helloworld has CLI component，try aos make helloworld@stm32f103rb-nucleo。
my board is Nucleo-F103RB，using PA2 and PA3 for UART log。If your board is a Nucleo board，you can use it directly。

btw，don't set breakpoint at the main() in the board/stm32f103rb-nucleo/Src/main.c，the real entry point is main() in platform\mcu\stm32l4xx_cube\aos\aos.c.

@walker2048 thanks for your information. will check it and come back.

@walker2048 check out the branch of F103RB. seem like somes files in platform/mcu/stm32f1xx_cube/aos/ are missed, would you check it and commti as well?
Thanks.

@mullife I've checked it.  Sorry about my mistack.
It seem's like git ignored aos/* files both in the board/ and plarform/ .

@walker2048 If I want to implement this code onto my own board, which part should I modify? Thanks

@walker2048 thanks

@chennanxu 不好意思，看错了。。。

@mullife
I had double check  the code，pull it from git again.
Helloworld app with CLI complie success，and run on my nucleo board with no problem.

@walker2048  Appreciate so much for your detail information. i will check it and feedback you.

@walker2048 Greate job!!!!. check your porting and now i can get UART log from PA9 & PA10 on STM32F103C8T6 (my board is only working with USART1). I will check why is not working on STM32F103VET6 for my previous porting.

@walker2048 As mentioned as above, based on your porting, chang port from USART 2 to USART 1(PA9 & PA10), it's able to print the log by printf from UART1.  but look like it's not able to get the input from UART1 and then CLI is not able to get char.

USART1_IRQHandler doesn't happen enven i input some char by chat.
Stuck in krhino_buf_queue_recv().

Have the following changes to switch the UART port from USART 2 to USART 1

hal_uart_stm32f1.h
a). Change the definition
#define PORT_UART1 0           /* USART1 /
#define PORT_UART2 1           / the port number of stand uart must be 0 */
b). change the GPIO/CLK definition in this file for USART1.
Change the GPIO initialization in stm32f1xx_hal_msp.c for USART 1 GPIO.

Do you meet the same issue that CLI is not able to get the char for TX?
thanks.

@walker2048 Yes, once receive the data, UART1 Intterruption will be trigged and HAL_UART_RxCpltCallback()  will be called then data will be sent to queue buffer by krhino_buf_queue_send()
Actually i have added the log in USART1_IRQHandler() and HAL_UART_RxCpltCallback(). but the log is never printed.
So check PA10(RX) state and it's always low even i put some char from laptop. so suppose that PA10 is not configured correctly. But check code with some time and didn't find any suspicion
it's sample to configured UART

hal_uart_init()
UART1_TX_GPIO_CLK_ENABLE();
UART1_RX_GPIO_CLK_ENABLE();
UART1_CLK_ENABLE();
HAL_NVIC_SetPriority(UART1_IRQn, 0, 1);
HAL_NVIC_EnableIRQ(UART1_IRQn);
HAL_UART_MspInit()
GPIO_InitStruct.Pin = GPIO_PIN_9|GPIO_PIN_10;
GPIO_InitStruct.Mode = GPIO_MODE_AF_PP;
GPIO_InitStruct.Pull = GPIO_NOPULL;
GPIO_InitStruct.Speed = GPIO_SPEED_FREQ_HIGH;
HAL_GPIO_Init(GPIOA, &GPIO_InitStruct);

@mullife
The LOG function won't work in the IRQHandler functions.  I try to LOG in gpio_isr_handler(),  or uart irqhandler,  the log is never printed.  Why？ I didn't figure it out.
If you only modify the pinset of USART2 in hal_uart_stm32f1.h  , can it work?
Change GPIO_PIN_2 to  GPIO_PIN_9 ,  GPIO_PIN_3 to GPIO_PIN_10
I can try to use USART1 on pin PA9 and PA10 tomorrow.

@mullife
hal_uart_init()
UART1_TX_GPIO_CLK_ENABLE();       --- __HAL_RCC_GPIOB_CLK_ENABLE()    should be GPIOA
UART1_RX_GPIO_CLK_ENABLE();       --- __HAL_RCC_GPIOB_CLK_ENABLE()    should be GPIOA
UART1_CLK_ENABLE();
HAL_NVIC_SetPriority(UART1_IRQn, 0, 1);
HAL_NVIC_EnableIRQ(UART1_IRQn);
But in your case, it can print log, the GPIOA clock is enable.
I will check it tomorrow.

@mullife
Modify the GPIO init config in HAL_UART_MspInit(), will fix this problem.
Create by STM32CubeMX:
    GPIO_InitStruct.Pin = GPIO_PIN_9;
    GPIO_InitStruct.Mode = GPIO_MODE_AF_PP;
    GPIO_InitStruct.Speed = GPIO_SPEED_FREQ_HIGH;
    HAL_GPIO_Init(GPIOA, &GPIO_InitStruct);

    GPIO_InitStruct.Pin = GPIO_PIN_10;
    GPIO_InitStruct.Mode = GPIO_MODE_INPUT;
    GPIO_InitStruct.Pull = GPIO_NOPULL;
    HAL_GPIO_Init(GPIOA, &GPIO_InitStruct);

@walker2048 Greate and change the mode to GPIO_MODE_INPUT and CLI is working fine now.
I thought i had this change for checking with GPIO_MODE_AF_INPUT which is the same as GPIO_MODE_INPUT
Anyway, it's greate support and Appreciate your help freely.
And now my STM32F103VET6 is also boot up.  The flash size of STM32F103C8T6 is too small for Alios-Things. So will have to STM32F103VET6

@walker2048 Also share the porting changes by https://github.com/mullife/AliOS-Things/tree/rel_1.3.3_stm32f1xx_cube.
Thanks.

@mullife My pleasure.


Hud Not Centered

So when testing in the simulator and in my app SVProgressHUD fails to be centered correctly. The suspecting line is line 660 --->
.....
} else {
keyboardHeight = self.visibleKeyboardHeight;
}
of SVProgressHud.m
Apparently this is doing a call to search the window subviews for a keyboard and is factoring in a keyboard height into the centering. The problem is this is returning a keyboard height when there is no keyboard on the screen so I'd suggest refactoring that method to do better detection.

I see you used RKeyboardListener in you clone. However, we can not integrate an additional third-party lib. Do you think it would be a good idea to "transfer" the code to this one?

Hey @honkmaster: Yes I have no problem if you guys want to just copy that. Unfortunately when looking at the design of SVProgressHUD there was no way to know the size of the keyboard as soon as the hud gets displayed hence the reason why a listener has to be installed and running in the app. Considering how lightweight the listener is and the added advantages (being able to detect if the keyboard is shown and its size appwide) I decided this method is better than attempting to identify the keyboard view in the window hierarchy which is hacky..

Same issue here.


TypeError: Cannot read property 'mark' of undefined

The Master branch seems to be throwing an error that I'm unable to track down exactly.

The good news is that I was able to fork the repository, merge develop into master, run the yarn run bundle command and get past the problem by including my fork as a dependency.
Here's the screen capture video of my process, adding the library to a fresh create-react-app project.
Note, create-react-app uses webpack and ES6 by default.

Hey,
I don't really understand this problem, is it still  problem ? if yes can you give more informations ?
Thanks

Hmmm, I'm trying to remember what this issue was... I think it had something to do with trying to add the dependency with the command:
yarn add wavesjs/waves-ui
I don't think the dev branch was working for me due to the annotated-segment issue here:
https://github.com/wavesjs/waves-ui/issues/25
If the develop branch is fixed then this can probably be closed out. I'll try and test this morning and report back.

Ok, the problem here stems from the fact that the develop branch does not include the build files required when importing the library using ES6.
import * as wavesUI from 'waves-ui';
The DEV branch is required due to all of the improvements and issues detailed here:
https://github.com/wavesjs/waves-ui/issues/25
So, as far as I can tell, including this project as a dependency, using npm or yarn,  is currently broken. I was able to get around this issue by forking the repository and build the project myself, incorporating the changes in the develop branch.
The line in my package.json looks like this:
 "waves-ui": "https://github.com/justinlevi/waves-ui.git"
My complete package.json:
{
  "name": "my project",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "bootstrap-sass": "^3.3.7",
    "node-sass-chokidar": "^0.0.3",
    "npm-run-all": "^4.1.1",
    "prop-types": "^15.6.0",
    "react": "^16.0.0",
    "react-bootstrap": "^0.31.3",
    "react-dom": "^16.0.0",
    "react-redux": "^5.0.6",
    "react-scripts": "1.0.14",
    "redux": "^3.7.2",
    "uuid": "^3.1.0",
    "waves-loaders": "wavesjs/waves-loaders",
    "waves-ui": "https://github.com/justinlevi/waves-ui.git"
  },
  "scripts": {
    "build-css": "node-sass-chokidar --include-path ./src/styles --include-path ./node_modules src/styles/ -o src/styles/",
    "watch-css": "npm run build-css && node-sass-chokidar --include-path ./src/styles --include-path ./node_modules src/styles/ -o src/styles --watch --recursive",
    "start-js": "react-scripts start",
    "start": "npm-run-all -p watch-css start-js",
    "build": "npm run build-css && react-scripts build",
    "test": "react-scripts test --env=jsdom",
    "eject": "react-scripts eject"
  }
}

I don't think this will be an issue once you merge develop into master and rebuild.

Yes it looks like this is the problem, I will try to make a release soon to prevent this problem.


Ldbruce patch 2

Hi Liana,
The changes you are proposing here are for the demo repository itself, not your own fork (copy) of it. Please fork the repository and then apply your changes to your fork, where you won't need my approval to merge them. Let me know if you need any help with that. If you are interested in an easier way of creating a webpage with bibbase, you can also try one of our premium plans, which include a hosted website with an easy to use editor and where embedding publication lists is trivial.
Best,
Christian


OpenVino Mac?

Just wondering why openvino on mac wasn't implemented did it not result in a similar speedup?

@cchance27 I don't have a Mac machine to test, if you can check it, please let me know.
https://github.com/rupeshs/fastsdcpu#fastsd-cpu-on-mac

@cchance27 Are you using M1?

FYI on my 2015 Mac with 2.5 GHz Quad-Core Intel Core i7, this works and enables openvino

FYI on my 2015 Mac with 2.5 GHz Quad-Core Intel Core i7, this works and enables openvino

Thanks for trying FastSD CPU


Support dart 1.13 (re: SecureSocket)

http_server needs to be updated here and there to support Dart 1.13:

test/utils.dart needs to not call SecureSocket.initialize.
Other problems in tests, I think.

Dart has provided a migration guide. Um... it might be out of date though. SecureSocket.initialize doesn't exist anymore.

Great call on the migration guide, @srawlins – opened a bug on the site – https://github.com/dart-lang/www.dartlang.org/issues/1520

Fixed in 3ed8c55b96fad5854a6d0bc419c446ee0f84303a.