Skip to content

Conversation

stmatengss
Copy link
Collaborator

Followup of #738

@stmatengss stmatengss force-pushed the add_master_side_cal branch from db19705 to 5d6c4a2 Compare August 14, 2025 04:28
@stmatengss
Copy link
Collaborator Author

@SgtPepperr could you review this PR?

return tl::make_unexpected(ErrorCode::INTERNAL_ERROR);
}
return PingResponse(view_version_, client_status);
return PingResponse(view_version_, client_status, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like qp_count should be passed in as a parameter here?

MasterMetricManager::instance().inc_ping_requests();

auto result = master_service_.Ping(client_id);
auto result = master_service_.Ping(client_id, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like qp_count should be passed in as a parameter here?

// Increment cluster total QP number based on client's reported QP count
MasterMetricManager::instance().inc_cluster_total_qp_num(
result.value().total_qp_num);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation increments QP count on every ping operation which may cause inaccurate counting, so maybe we should maintain a map<uuid, qpcount> in master_metrics to update each client's status during pings and sum all qpcounts when queried.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants