Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cluster info sent stats for message with light header #1563

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

hpatro
Copy link
Collaborator

@hpatro hpatro commented Jan 15, 2025

This issue only impacted two message type (CLUSTERMSG_TYPE_PUBLISH / CLUSTERMSG_TYPE_PUBLISHSHARD) as those were the only message type uses light message header where the CLUSTER INFO stats had missing information of sent/received messages of that type.

Before

cluster_state:ok
cluster_slots_assigned:16384
cluster_slots_ok:16384
cluster_slots_pfail:0
cluster_slots_fail:0
cluster_known_nodes:3
cluster_size:3
cluster_current_epoch:2
cluster_my_epoch:0
cluster_stats_messages_ping_sent:20
cluster_stats_messages_pong_sent:22
cluster_stats_messages_meet_sent:2
cluster_stats_messages_sent:44
cluster_stats_messages_ping_received:22
cluster_stats_messages_pong_received:22
cluster_stats_messages_received:44
total_cluster_links_buffer_limit_exceeded:0

After

cluster_state:ok
cluster_slots_assigned:16384
cluster_slots_ok:16384
cluster_slots_pfail:0
cluster_slots_fail:0
cluster_known_nodes:3
cluster_size:3
cluster_current_epoch:2
cluster_my_epoch:1
cluster_stats_messages_ping_sent:20
cluster_stats_messages_pong_sent:23
cluster_stats_messages_meet_sent:2
**cluster_stats_messages_publish_sent:2**
cluster_stats_messages_sent:47
cluster_stats_messages_ping_received:23
cluster_stats_messages_pong_received:22
cluster_stats_messages_received:45
total_cluster_links_buffer_limit_exceeded:0

@hpatro hpatro requested a review from zuiderkwast January 15, 2025 05:10
@hpatro
Copy link
Collaborator Author

hpatro commented Jan 15, 2025

@zuiderkwast let me know if we can get the type without modifying the method signature.

@hpatro hpatro force-pushed the fix_lighthdr_sent_stats branch from aefef81 to 54a762f Compare January 15, 2025 05:17
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.95%. Comparing base (d99457c) to head (88d14a3).
Report is 18 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1563      +/-   ##
============================================
+ Coverage     70.94%   70.95%   +0.01%     
============================================
  Files           120      121       +1     
  Lines         65004    65130     +126     
============================================
+ Hits          46116    46215      +99     
- Misses        18888    18915      +27     
Files with missing lines Coverage Δ
src/cluster_legacy.c 87.25% <100.00%> (+0.34%) ⬆️

... and 24 files with indirect coverage changes

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Good idea. I replies to your question in a comment below.

We don't need to distinguish the heavy/light variants in the stats counter, because we have some other stats counting the bytes?

src/cluster_legacy.c Outdated Show resolved Hide resolved
@hpatro
Copy link
Collaborator Author

hpatro commented Jan 15, 2025

Good idea. I replies to your question in a comment below.

We don't need to distinguish the heavy/light variants in the stats counter, because we have some other stats counting the bytes?

I initially thought of adding light_{type}_sent/light_{type}_received and keep the messages with regular hdr stats as is. I felt it's not that useful for Valkey users but helps with debugging for us. WDYT ?

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

I initially thought of adding light_{type}_sent/light_{type}_received and keep the messages with regular hdr stats as is. I felt it's not that useful for Valkey users but helps with debugging for us. WDYT ?

I agree it's not useful for users. The light/heavy message type is an implementation detail. The bytes sent/received over the cluster bus can be interesting for users. We have that? I can't see it in the CLUSTER INFO man page.

Shall we mention it as a bugfix in the release notes? Backport to 8.0?

@hpatro
Copy link
Collaborator Author

hpatro commented Jan 15, 2025

I initially thought of adding light_{type}_sent/light_{type}_received and keep the messages with regular hdr stats as is. I felt it's not that useful for Valkey users but helps with debugging for us. WDYT ?

I agree it's not useful for users. The light/heavy message type is an implementation detail. The bytes sent/received over the cluster bus can be interesting for users. We have that? I can't see it in the CLUSTER INFO man page.

Do you mean the CLUSTER LINKS thing ? I don't see a sent/received stats though. We should add it I guess.

Shall we mention it as a bugfix in the release notes? Backport to 8.0?

Yeah, I think we should backport it to 8.

@zuiderkwast
Copy link
Contributor

Do you mean the CLUSTER LINKS thing ? I don't see a sent/received stats though. We should add it I guess.

I wasn't aware of CLUSTER LINK. Thanks. Looks like it shows only currently connected nodes. I guess we could add sent/received bytes there.

I was thinking of a total sent/received over the cluster bus, probably in CLUSTER INFO.

Perhaps we should wait until anyone actually requests it. :)

@zuiderkwast zuiderkwast added bug Something isn't working release-notes This issue should get a line item in the release notes cluster labels Jan 15, 2025
Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

LGTM.

tests/unit/cluster/pubsub.tcl Outdated Show resolved Hide resolved
hpatro and others added 3 commits January 16, 2025 19:08
Co-authored-by: Binbin <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
@hpatro hpatro force-pushed the fix_lighthdr_sent_stats branch from d5dd58e to 88d14a3 Compare January 16, 2025 19:09
@hpatro hpatro merged commit 87cc3d7 into valkey-io:unstable Jan 16, 2025
50 checks passed
hpatro added a commit to hpatro/valkey that referenced this pull request Jan 16, 2025
…1563)

This issue affected only two message types (CLUSTERMSG_TYPE_PUBLISH and CLUSTERMSG_TYPE_PUBLISHSHARD) because they used a light message header, which caused the CLUSTER INFO stats to miss sent/received message information for those types.

---------

Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Binbin <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
@hpatro
Copy link
Collaborator Author

hpatro commented Jan 16, 2025

Backport to 8.0 PR: #1571

proost pushed a commit to proost/valkey that referenced this pull request Jan 17, 2025
…1563)

This issue affected only two message types (CLUSTERMSG_TYPE_PUBLISH and CLUSTERMSG_TYPE_PUBLISHSHARD) because they used a light message header, which caused the CLUSTER INFO stats to miss sent/received message information for those types.

---------

Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Binbin <[email protected]>
Signed-off-by: proost <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cluster release-notes This issue should get a line item in the release notes
Projects
Status: To be backported
Development

Successfully merging this pull request may close these issues.

3 participants