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

CBG-3686 add idle kv ops stat #6642

Merged
merged 2 commits into from
Jan 17, 2024
Merged

CBG-3686 add idle kv ops stat #6642

merged 2 commits into from
Jan 17, 2024

Conversation

torcolvin
Copy link
Collaborator

Setup up two SG nodes in two modes and compared stat on server for idle ops:

  • ISGR between two nodes with different couchbase servers
  • two nodes serving same database (to shard DCP)

CfgSg operations will only show up if there's multiple SGs.

I had uncertainty about what level to put the calls, especially GetMetadataDocument. In this case, some idle operations are going to get called. I could move this to the bootstrap polling when we call GetDatabaseConfigs. On the other hand, I could move this into config_persistentence.go and cover xattr and non xattr case.

For heartbeater, I put the Get cases, but not the updating the node lists, which presumably happens on a rebalance. I think this is most correct.

For CfgSg, I added for Get operations, since again Set will happen for ISGR or cbgt DCP rebalance.

The concerns I have are as follows:

  • It is really easy to regress on this behavior because I did manual testing only and if we change pathways of code, this will break.
  • I think overcounting is OK when we are doing other operations (like updating a configuration file), because then it won't register as idle anyway, there will be more CRUD operations.
  • We are going to call this stat function a few times a second - is this updating the stat too frequently? I think it's fine because it is an atomic.

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

@torcolvin torcolvin requested a review from adamcfraser January 12, 2024 18:25
Copy link
Collaborator

@adamcfraser adamcfraser left a comment

Choose a reason for hiding this comment

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

Question about the cbgt ops, and suggestion to move the stat incr to after the op completes.

@@ -280,6 +280,7 @@ func (h *couchbaseHeartBeater) checkStaleHeartbeats(ctx context.Context) error {
continue
}

SyncGatewayStats.GlobalStats.ResourceUtilizationStats().NumIdleKvOps.Add(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we generally want to increment the stat after the operation is performed, not before. Doing it before the error check is fine if that's what you were trying to avoid.

@@ -95,8 +95,8 @@ func (c *CfgSG) Set(cfgKey string, val []byte, cas uint64) (uint64, error) {
return 0, fmt.Errorf("cfg_sg: key cannot start with a colon")
}

SyncGatewayStats.GlobalStats.ResourceUtilizationStats().NumIdleKvOps.Add(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The PR description said you added this stat for CfgSG.Get, but you've got it on Set here.

Having said that - what Cfg operations are actually being performed by cbgt when the DCP feed is idle? I had thought it was notified about cfg changes via Subscribe(), and didn't do polling.

Remove unused stat for CfgSG.
Add docs
@adamcfraser adamcfraser merged commit 87cca79 into master Jan 17, 2024
34 checks passed
@adamcfraser adamcfraser deleted the CBG-3686 branch January 17, 2024 22:02
bbrks pushed a commit that referenced this pull request Mar 28, 2024
* CBG-3686 add idle kv ops stat

* Move stats to after read/write operation.

Remove unused stat for CfgSG.
Add docs
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