Copilot commented on code in PR #2049:
URL:
https://github.com/apache/incubator-pegasus/pull/2049#discussion_r3744490327
##########
src/meta/meta_bulk_load_service.cpp:
##########
@@ -434,7 +438,7 @@ void bulk_load_service::partition_bulk_load(const
std::string &app_name, const g
const app_bulk_load_info &ainfo =
_app_bulk_load_info[pid.get_app_id()];
req->pid = pid;
req->app_name = app_name;
- SET_IP_AND_HOST_PORT(*req, primary, pc.primary, pc.hp_primary);
+ SET_OBJ_IP_AND_HOST_PORT(*req, primary, pc, primary);
req->remote_provider_name = ainfo.file_provider_type;
Review Comment:
`SET_OBJ_IP_AND_HOST_PORT(*req, primary, pc, primary)` assumes
`pc.hp_primary` is set and consistent with `pc.primary` (it DCHECKs on
`pc.primary == pc.hp_primary.resolve()`). In rolling-upgrade scenarios this
function explicitly tolerates `pc.__isset.hp_primary == false`, so this can
trip DCHECK or propagate an unset/invalid `hp_primary` into the request.
##########
src/replica/replica_stub.cpp:
##########
@@ -1200,19 +1200,32 @@ void replica_stub::on_config_proposal(const
configuration_update_request &propos
enum_to_string(proposal.type),
FMT_HOST_PORT_AND_IP(proposal, node));
- replica_ptr rep = get_replica(proposal.config.pid);
+ // Normalize the partition_configuration type 'config' before using it.
+ configuration_update_request normalized_proposal = proposal;
+ if (!normalized_proposal.config.__isset.hp_primary) {
+ dsn::host_port primary;
+ GET_HOST_PORT(normalized_proposal.config, primary, primary);
+ normalized_proposal.config.__set_hp_primary(primary);
+ }
Review Comment:
This normalization unconditionally calls `__set_hp_primary(primary)` even
when the derived `primary` is invalid (e.g., when `config.primary` is invalid).
That sets `__isset.hp_primary = true` with an invalid value, and later
`GET_HOST_PORT` will take the `__isset` branch and DCHECK that `config.primary`
is valid and matches `hp_primary.resolve()`, which can crash debug builds.
##########
src/meta/test/balancer_simulator/balancer_simulator.cpp:
##########
@@ -113,10 +113,12 @@ void generate_balanced_apps(/*out*/ app_mapper &apps,
for (auto &pc : app->pcs) {
temp.clear();
- while (pc.hp_secondaries.size() + 1 < pc.max_replica_count) {
+ std::vector<dsn::host_port> secondaries;
+ GET_HOST_PORTS(pc, secondaries, secondaries);
+ while (secondaries.size() + 1 < pc.max_replica_count) {
const auto &n = pq2.pop();
if (!is_member(pc, n)) {
- pc.hp_secondaries.push_back(n);
+ secondaries.push_back(n);
nodes[n].put_partition(pc.pid, false);
Review Comment:
`GET_HOST_PORTS(pc, secondaries, secondaries)` populates a local vector, but
any changes to `secondaries` are never written back to `pc`. As a result,
`pc.hp_secondaries` / `pc.secondaries` stay unchanged (likely empty), so the
generated app is missing secondaries even though `nodes[n].put_partition(...)`
is updated.
##########
src/meta/meta_data.h:
##########
@@ -487,10 +504,16 @@ inline config_context *get_config_context(app_mapper
&apps, const dsn::gpid &gpi
return &(iter->second->helpers->contexts[gpid.get_partition_index()]);
}
-inline int replica_count(const partition_configuration &pc)
+inline size_t replica_count(const partition_configuration &pc)
{
- int ans = pc.hp_primary ? 1 : 0;
- return ans + pc.hp_secondaries.size();
+ host_port primary;
+ GET_HOST_PORT(pc, primary, primary);
+ size_t rc = primary ? 1 : 0;
+
+ std::vector<host_port> secondaries;
+ GET_HOST_PORTS(pc, secondaries, secondaries);
+ rc += secondaries.size();
+ return rc;
Review Comment:
`replica_count` allocates and copies the entire secondaries vector via
`GET_HOST_PORTS(...)` just to compute `.size()`. This adds avoidable
allocations/copies in meta hot paths (e.g., balancing / config checks) and is
unnecessary because the count can be computed from `pc.hp_secondaries.size()`
when `__isset` is true, otherwise from `pc.secondaries.size()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
