Skip to content

HBASE-25596: Fix NPE and avoid permanent unreplicated data due to EOF - #2975

Merged
apurtell merged 3 commits into
apache:branch-1from
sandeepvinayak:HBASE-25596
Feb 23, 2021
Merged

HBASE-25596: Fix NPE and avoid permanent unreplicated data due to EOF#2975
apurtell merged 3 commits into
apache:branch-1from
sandeepvinayak:HBASE-25596

Conversation

@sandeepvinayak

@sandeepvinayaksandeepvinayak commented Feb 22, 2021

Copy link
Copy Markdown
Contributor

sandeepvinayak
sandeepvinayak commented Feb 22, 2021
View reviewed changes
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java Outdated
// After we removed the WAL from the queue, we should
// try shipping the existing batch of entries, we do not want to reset
// stream since entry stream doesn't have the correct data at this point
isBatchQueuedToBeShipped(entryStream, batch, hasMoreData, true);

@sandeepvinayaksandeepvinayakFeb 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is the fix to avoid permanent data loss.

apurtell
apurtell requested changes Feb 22, 2021
View reviewed changes
Comment thread...server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
Comment thread...server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java Outdated
// If this is a recovered queue, the queue is already full and the first log
// normally has a position (unless the RS failed between 2 logs)
private long getRecoveredQueueStartPos(long startPosition) {
public long getRecoveredQueueStartPos(long startPosition) {

@apurtellapurtellFeb 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this change needed? I don't see any changes to a caller of this method in this patch

@sandeepvinayaksandeepvinayakFeb 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, I will revert it.

Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment threadhbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSource.java
Comment threadhbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSource.java
Comment thread...erver/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestWALEntryStream.java
assertEquals(0, queue.size());
}

@Test

@apurtellapurtellFeb 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice test

xcangCRM
xcangCRM reviewed Feb 23, 2021
View reviewed changes
Comment threadhbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSource.java
Comment thread...erver/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestWALEntryStream.java
queue, 0, fs, conf, getDummyFilter(),
new MetricsSource("1"), (ReplicationSource) source);
reader.run();

@xcangCRMxcangCRMFeb 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do you need to do some wait here? How did you drain the queue? I might be missing something here. thanks

@sandeepvinayaksandeepvinayakFeb 23, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@xcangCRM i think you are confusing it with .start() method which is async call. We are executing .run() which is sync call.

Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java Outdated
* @throws InterruptedException throws interrupted exception
* @throws IOException throws io exception from stream
*/
private boolean updateBatchAndAddInShippingQueue(WALEntryStream entryStream, WALEntryBatch batch,

@xcangCRMxcangCRMFeb 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit:
name is bit long,
how about updateBatchAndShippingQueue

Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Comment thread.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
@Apache-HBase

Apache-HBase commented Feb 23, 2021

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec3m 52sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 3 new or modified test files.
_ branch-1 Compile Tests _
+1 💚mvninstall11m 17sbranch-1 passed
+1 💚compile0m 59sbranch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19
+1 💚compile1m 0sbranch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10
+1 💚checkstyle2m 19sbranch-1 passed
+1 💚shadedjars4m 33sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 7sbranch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19
+1 💚javadoc1m 0sbranch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10
+0 🆗spotbugs4m 13sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs4m 8sbranch-1 passed
_ Patch Compile Tests _
+1 💚mvninstall2m 51sthe patch passed
+1 💚compile1m 3sthe patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19
+1 💚javac1m 3sthe patch passed
+1 💚compile1m 4sthe patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10
+1 💚javac1m 4sthe patch passed
+1 💚checkstyle2m 14shbase-server: The patch generated 0 new + 0 unchanged - 10 fixed = 0 total (was 10)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚shadedjars4m 40spatch has no errors when building our shaded downstream artifacts.
+1 💚hadoopcheck6m 48sPatch does not cause any errors with Hadoop 2.8.5 2.9.2.
+1 💚javadoc0m 57shbase-server-jdkAzulSystems,Inc.-1.8.0_262-b19 with JDK Azul Systems, Inc.-1.8.0_262-b19 generated 0 new + 6 unchanged - 2 fixed = 6 total (was 8)
+1 💚javadoc1m 3shbase-server-jdkAzulSystems,Inc.-1.7.0_272-b10 with JDK Azul Systems, Inc.-1.7.0_272-b10 generated 0 new + 6 unchanged - 2 fixed = 6 total (was 8)
+1 💚findbugs4m 37sthe patch passed
_ Other Tests _
-1 ❌unit150m 16shbase-server in the patch failed.
+1 💚asflicense0m 29sThe patch does not generate ASF License warnings.
211m 35s
ReasonTests
Failed junit testshadoop.hbase.regionserver.TestSplitTransactionOnCluster
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2975/1/artifact/out/Dockerfile
GITHUB PR#2975
JIRA IssueHBASE-25596
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux f81601d65ef1 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-2975/out/precommit/personality/provided.sh
git revisionbranch-1 / f9a9148
Default JavaAzul Systems, Inc.-1.7.0_272-b10
Multi-JDK versions/usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10
unithttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2975/1/artifact/out/patch-unit-hbase-server.txt
Test Resultshttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2975/1/testReport/
Max. process+thread count3660 (vs. ulimit of 10000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2975/1/console
versionsgit=1.9.1 maven=3.0.5 findbugs=3.0.1
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Feb 23, 2021

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec2m 38sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 1sThe patch does not contain any @author tags.
+1 💚test4tests0m 0sThe patch appears to include 3 new or modified test files.
_ branch-1 Compile Tests _
+1 💚mvninstall10m 48sbranch-1 passed
+1 💚compile1m 8sbranch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19
+1 💚compile1m 6sbranch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10
+1 💚checkstyle2m 28sbranch-1 passed
+1 💚shadedjars4m 28sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc1m 3sbranch-1 passed with JDK Azul Systems, Inc.-1.8.0_262-b19
+1 💚javadoc0m 56sbranch-1 passed with JDK Azul Systems, Inc.-1.7.0_272-b10
+0 🆗spotbugs4m 21sUsed deprecated FindBugs config; considering switching to SpotBugs.
+1 💚findbugs4m 18sbranch-1 passed
-0 ⚠️patch4m 28sUsed diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
_ Patch Compile Tests _
+1 💚mvninstall2m 48sthe patch passed
+1 💚compile0m 59sthe patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19
+1 💚javac0m 59sthe patch passed
+1 💚compile1m 1sthe patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10
+1 💚javac1m 1sthe patch passed
+1 💚checkstyle2m 16shbase-server: The patch generated 0 new + 0 unchanged - 10 fixed = 0 total (was 10)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚shadedjars4m 6spatch has no errors when building our shaded downstream artifacts.
+1 💚hadoopcheck5m 56sPatch does not cause any errors with Hadoop 2.8.5 2.9.2.
+1 💚javadoc0m 33sthe patch passed with JDK Azul Systems, Inc.-1.8.0_262-b19
+1 💚javadoc0m 43sthe patch passed with JDK Azul Systems, Inc.-1.7.0_272-b10
+1 💚findbugs3m 9sthe patch passed
_ Other Tests _
+1 💚unit122m 57shbase-server in the patch passed.
+1 💚asflicense0m 31sThe patch does not generate ASF License warnings.
178m 15s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2975/3/artifact/out/Dockerfile
GITHUB PR#2975
JIRA IssueHBASE-25596
Optional Testsdupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile
unameLinux 471c6b614fb2 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personality/home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-2975/out/precommit/personality/provided.sh
git revisionbranch-1 / 9e9301a
Default JavaAzul Systems, Inc.-1.7.0_272-b10
Multi-JDK versions/usr/lib/jvm/zulu-8-amd64:Azul Systems, Inc.-1.8.0_262-b19 /usr/lib/jvm/zulu-7-amd64:Azul Systems, Inc.-1.7.0_272-b10
Test Resultshttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2975/3/testReport/
Max. process+thread count3693 (vs. ulimit of 10000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2975/3/console
versionsgit=1.9.1 maven=3.0.5 findbugs=3.0.1
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@apurtell

apurtell commented Feb 23, 2021

Copy link
Copy Markdown
Contributor

Unless there is a change in review status I will merge this in a couple of hours.

@apurtell
apurtell merged commit 19fe18e into apache:branch-1Feb 23, 2021
This was referenced Feb 25, 2021
Merged
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apurtellapurtellapurtell approved these changes
@xcangCRMxcangCRMxcangCRM approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sandeepvinayak@Apache-HBase@apurtell@xcangCRM