HBASE-27463 Reset sizeOfLogQueue when refresh replication source #4863
Conversation
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| @@ -467,6 +467,9 @@ public void refreshSources(String peerId) throws IOException { | |||
| ReplicationSourceInterface toRemove = this.sources.put(peerId, src); | |||
| if (toRemove != null) { | |||
| LOG.info("Terminate replication source for " + toRemove.getPeerId()); | |||
| // Reset sizeOfLogQueue, log will re enqueue to the created new source. | |||
| toRemove.getSourceMetrics() | |||
There was a problem hiding this comment.
@frostruan Do you think we need to reset just size of log queue metric or other source metrics also? IMO we should reset all the metrics and start with clean state?
There was a problem hiding this comment.
Thanks for reviewing @shahrs87
Currently we monitor the replication status by the sizeOfLogQueue metric, so I just reset the sizeOfLogQueue here. I'll check if there is any other metrics need to be reset. Thanks for your suggestion.
There was a problem hiding this comment.
Are there any rules about which metrics should be reset while others should not?
There was a problem hiding this comment.
Thanks for review Duo.@Apache9
Sorry, currently have no idea about the rule you mentioned, maybe the rule is a little complicated.
I think what makes the rule complicated is that we first create the new ReplicationSource, then replace the old ReplicationSource with the new one, and then if the old ReplicationSource exists, terminate it. And since HBASE-23231, we will not clear the old metrics when terminate the old ReplicationSource to avoid the metric for the new ReplicationSource being cleared. Then it's a little complicated to keep the new ReplicationSourceMetric and the GlobalReplicationSourceMetric right and consistent.
If we adjust the order, first terminate the old ReplicationSource if it exists and then create and register the new ReplicationSource, the logic of metric here maybe will be much simpler, and of course we can clear the metric when terminate the old ReplicationSource. But I'm not sure yet, still need to confirm
There was a problem hiding this comment.
This change will effectively undo HBASE-23231 @Apache9 Since you were one of the reviewer on HBASE-23231, do you think it is safe to do this?
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
So the fix here is to create the new replication source after terminating the old replication source. First, how could this fix the problem? |
Thanks for reviewing Duo @Apache9 The problem arises like this
So I think maybe we can change the order, terminate old replication first and then create the new one. That will make the problem less complicated. |
OK, so if we terminate the replication source first, we do not need to keep the old metrics, just let it decrease and after we create the new replication source, the metrics will be restored? |
|
yes. I think so. |
|
@shahrs87 Do you have any other concerns? |
|
@frostruan Thank you for the update. I think this is more cleaner way. +1. |
Co-authored-by: huiruan <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Rushabh Shah <[email protected]> (cherry picked from commit bb9f43c)
Co-authored-by: huiruan <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Rushabh Shah <[email protected]> (cherry picked from commit bb9f43c)
Co-authored-by: huiruan <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Rushabh Shah <[email protected]> (cherry picked from commit bb9f43c)
…che#4863) Co-authored-by: huiruan <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Rushabh Shah <[email protected]> (cherry picked from commit bb9f43c) (cherry picked from commit 5ce1d8f) Change-Id: I9f8a19286df32d68de472e8f4b3f8f7926551178
No description provided.