HBASE-24871 Replication may loss data when refresh recovered replicat… #2249
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Add a UT for this case? |
|
🎊 +1 overall
This message was automatically generated. |
OK, I'll try it. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
As @infraio said, it will be great to add an UT to show the issue w/o change. Otherwise, looks good to me. |
huaxiangsun
requested changes
Aug 12, 2020
wchevreuil
reviewed
Aug 13, 2020
| @@ -516,7 +516,7 @@ public void refreshSources(String peerId) throws IOException { | |||
| ReplicationSourceInterface replicationSource = createSource(queueId, peer); | |||
| this.oldsources.add(replicationSource); | |||
| for (SortedSet<String> walsByGroup : walsByIdRecoveredQueues.get(queueId).values()) { | |||
| walsByGroup.forEach(wal -> src.enqueueLog(new Path(wal))); | |||
| walsByGroup.forEach(wal -> replicationSource.enqueueLog(new Path(wal))); | |||
There was a problem hiding this comment.
So the problem is that logs from recovered queues were getting added to the new "normal" queue? And these logs are never read, then?
Nit: maybe worth rename the variable from line #516 from replicationSource to recoveredReplicationSource, for further clarity?
I endorse the call for an additional UT, here.
There was a problem hiding this comment.
So the problem is that logs from recovered queues were getting added to the new "normal" queue? And these logs are never read, then?
Yes. These log will be added to normal source. And will read by normal source. So this should not loss data but only affect the replicate order.
@ddupg I thought the issue title need to be changed.
There was a problem hiding this comment.
Thanks for clarifying, @infraio . Yes, let's change the title, please.
There was a problem hiding this comment.
Thank @wchevreuil @infraio for reviewing. In this case, it will loss data and duplicate data at the same time.
After we added these log to normal source, the normal source replicate the first WAL entries batch and updateLogPosition via zk.
It will successfully replicate first WAL entries batch, but data may be out-of date. And then failed to updateLogPosition in zk, which aborting RS, so we will also loss data.
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Thank @huaxiangsun for reviewing, I've added an UT for this case. |
|
Thanks @ddupg. One quick question, will the new UT fail without changes in refreshSources()? Just want to make sure that the UT really does its job. +1 pending on your answer. |
huaxiangsun
approved these changes
Aug 20, 2020
Yes, the UT fail without changes in this patch. Because it failed to updateLogPosition in zk after replicating first WAL edits batch, which aborting RS. |
|
@infraio and @wchevreuil, any more comments before I merge the changes? Thanks. |
infraio
reviewed
Aug 22, 2020
| !replication.getReplicationManager().getOldSources().isEmpty()); | ||
|
|
||
| // disable peer to trigger refreshSources | ||
| hbaseAdmin.disableReplicationPeer(PEER_ID2); |
There was a problem hiding this comment.
Which assert will failed if no this fix?
There was a problem hiding this comment.
The UT will timeout in last UTIL2.waitFor without this fix because of losing data.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
infraio
pushed a commit
that referenced
this pull request
Aug 24, 2020
…ion sources (#2249) Signed-off-by: huaxiangsun <[email protected]> Signed-off-by: Guanghao Zhang <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
infraio
pushed a commit
that referenced
this pull request
Aug 24, 2020
…ion sources (#2249) Signed-off-by: huaxiangsun <[email protected]> Signed-off-by: Guanghao Zhang <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
infraio
pushed a commit
that referenced
this pull request
Aug 24, 2020
…ion sources (#2249) Signed-off-by: huaxiangsun <[email protected]> Signed-off-by: Guanghao Zhang <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…ion sources (apache#2249) Signed-off-by: huaxiangsun <[email protected]> Signed-off-by: Guanghao Zhang <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…ion sources (apache#2249) Signed-off-by: huaxiangsun <[email protected]> Signed-off-by: Guanghao Zhang <[email protected]> Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java (cherry picked from commit e300ae3) Change-Id: I5b2aa29597811d77ce99732019a2ac3217c93dab
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ion sources