Skip to content

HBASE-28090 Make entryReader field final in ReplicationSourceShipper … - #5409

Merged
Apache9 merged 1 commit into
apache:masterfrom
Apache9:HBASE-28090
Sep 18, 2023
Merged

HBASE-28090 Make entryReader field final in ReplicationSourceShipper …#5409
Apache9 merged 1 commit into
apache:masterfrom
Apache9:HBASE-28090

Conversation

@Apache9

Copy link
Copy Markdown
Contributor

…class

@Apache9Apache9 self-assigned this Sep 18, 2023
@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 27sDocker 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.
_ master Compile Tests _
+1 💚mvninstall3m 5smaster passed
+1 💚compile2m 35smaster passed
+1 💚checkstyle0m 35smaster passed
+1 💚spotless0m 45sbranch has no errors when running spotless:check.
+1 💚spotbugs1m 31smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 42sthe patch passed
+1 💚compile2m 34sthe patch passed
+1 💚javac2m 34sthe patch passed
+1 💚checkstyle0m 33sthe patch passed
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚hadoopcheck10m 28sPatch does not cause any errors with Hadoop 3.2.4 3.3.6.
+1 💚spotless0m 42spatch has no errors when running spotless:check.
+1 💚spotbugs1m 35sthe patch passed
_ Other Tests _
+1 💚asflicense0m 10sThe patch does not generate ASF License warnings.
34m 34s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#5409
Optional Testsdupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile
unameLinux a6a1ab40ad41 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / ef7b854
Default JavaEclipse Adoptium-11.0.17+8
Max. process+thread count77 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/console
versionsgit=2.34.1 maven=3.8.6 spotbugs=4.7.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 10sDocker mode activated.
-0 ⚠️yetus0m 3sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚mvninstall2m 48smaster passed
+1 💚compile0m 47smaster passed
+1 💚shadedjars4m 39sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 27smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 36sthe patch passed
+1 💚compile0m 47sthe patch passed
+1 💚javac0m 47sthe patch passed
+1 💚shadedjars4m 38spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 24sthe patch passed
_ Other Tests _
+1 💚unit244m 5shbase-server in the patch passed.
266m 15s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR#5409
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 7d39ca17965d 5.4.0-156-generic #173-Ubuntu SMP Tue Jul 11 07:25:22 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / ef7b854
Default JavaEclipse Adoptium-11.0.17+8
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/testReport/
Max. process+thread count4896 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 32sDocker mode activated.
-0 ⚠️yetus0m 4sUnprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+1 💚mvninstall2m 25smaster passed
+1 💚compile0m 39smaster passed
+1 💚shadedjars4m 31sbranch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 25smaster passed
_ Patch Compile Tests _
+1 💚mvninstall2m 14sthe patch passed
+1 💚compile0m 40sthe patch passed
+1 💚javac0m 40sthe patch passed
+1 💚shadedjars4m 29spatch has no errors when building our shaded downstream artifacts.
+1 💚javadoc0m 23sthe patch passed
_ Other Tests _
+1 💚unit255m 8shbase-server in the patch passed.
276m 5s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR#5409
Optional Testsjavac javadoc unit shadedjars compile
unameLinux 531441631a55 5.4.0-152-generic #169-Ubuntu SMP Tue Jun 6 22:23:09 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / ef7b854
Default JavaTemurin-1.8.0_352-b08
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/testReport/
Max. process+thread count4157 (vs. ulimit of 30000)
modulesC: hbase-server U: hbase-server
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5409/1/console
versionsgit=2.34.1 maven=3.8.6
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@wchevreuilwchevreuil left a comment

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.

So the aim here is to make wal reader final to avoid it from being set to null, inadvertently? I think the NPE condition reported in HBASE-28076 would not be possible in master branch code, because we are not trying to reuse RecoveredReplicationSourceShipper logic to read wal position (we now do it from ReplicationSource.getStartOffset), which will not call RecoveredReplicationSourceShipper.terminate in the event of failing to get WAL position).

@Apache9

Copy link
Copy Markdown
ContributorAuthor

So the aim here is to make wal reader final to avoid it from being set to null, inadvertently? I think the NPE condition reported in HBASE-28076 would not be possible in master branch code, because we are not trying to reuse RecoveredReplicationSourceShipper logic to read wal position (we now do it from ReplicationSource.getStartOffset), which will not call RecoveredReplicationSourceShipper.terminate in the event of failing to get WAL position).

Yes, we have a null check in ReplicationSource.terminate, which is not necessary, but since it is not final, it is possible that after changing the initialization order we hit the NPE again, so here I reactored a bit to make it final so it will never be null.

@wchevreuilwchevreuil left a comment

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.

Thanks for confirming. LGTM!

@Apache9
Apache9 merged commit 787d524 into apache:masterSep 18, 2023
Apache9 added a commit that referenced this pull request Sep 19, 2023
…class (#5409)
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
(cherry picked from commit 787d524)
wchevreuil pushed a commit to wchevreuil/hbase that referenced this pull request Nov 2, 2023
…class (apache#5409)
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
Sign up for freeto 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.

3 participants

@Apache9@Apache-HBase@wchevreuil