-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-27785 Encapsulate and centralize totalBufferUsed in Replication… #5168
Conversation
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -1070,4 +1070,21 @@ MetricsReplicationGlobalSourceSource getGlobalMetrics() { | |||
ReplicationQueueStorage getQueueStorage() { | |||
return queueStorage; | |||
} | |||
|
|||
boolean addTotalBufferUsed(long size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments to mention the meaning of the return value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After checking the later code, I think here we'd better introduce two methods, one is acquire, which returns a boolean, the other is release, which returns nothing. And they could both call a private method, where we change the totalBufferUsed, and record the new usage in metrics.
This will be much clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Apache9 ,thanks for suggestion, I would make it more clearer.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor naming issue. Can fix on commit.
Thanks @comnetwork for refactoring the polishing the code.
totalToDecrement.accumulate(entrySizeExcludeBulkLoad); | ||
}); | ||
}); | ||
long totalToDecrement = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should name it totalReleasedBytes or totalDecremented? As we do the decrement on the fly, the final number is just used for logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Apache9 , thanks for suggestion, already fixed it.
…SourceManager