Skip to content

Conversation

@ndimiduk
Copy link
Member

@ndimiduk ndimiduk commented Mar 7, 2022

Signed-off-by: Bharath Vissapragada [email protected]
Signed-off-by: Duo Zhang [email protected]

Signed-off-by: Bharath Vissapragada <[email protected]>
Signed-off-by: Duo Zhang <[email protected]>
@ndimiduk ndimiduk added the backport This PR is a back port of some issue or issues already committed to master label Mar 7, 2022
@ndimiduk
Copy link
Member Author

ndimiduk commented Mar 7, 2022

@stoty can you confirm that this change is not problematic for Phoenix?

@stoty
Copy link
Contributor

stoty commented Mar 7, 2022

Technically, Phoenix doesn't support branch-2, only branch-2.[1..4]
Let me check what happens if I apply this on branch-2.4.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 51s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ branch-2 Compile Tests _
+1 💚 mvninstall 2m 48s branch-2 passed
+1 💚 compile 2m 15s branch-2 passed
+1 💚 checkstyle 0m 37s branch-2 passed
+1 💚 spotbugs 1m 15s branch-2 passed
_ Patch Compile Tests _
+1 💚 mvninstall 2m 17s the patch passed
+1 💚 compile 2m 11s the patch passed
+1 💚 javac 2m 11s the patch passed
+1 💚 checkstyle 0m 37s hbase-server: The patch generated 0 new + 1 unchanged - 14 fixed = 1 total (was 15)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 hadoopcheck 7m 42s Patch does not cause any errors with Hadoop 3.1.2 3.2.1.
+1 💚 spotbugs 1m 18s the patch passed
_ Other Tests _
+1 💚 asflicense 0m 14s The patch does not generate ASF License warnings.
27m 16s
Subsystem Report/Notes
Docker ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4173/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #4173
Optional Tests dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
uname Linux f21069465056 5.4.0-1025-aws #25~18.04.1-Ubuntu SMP Fri Sep 11 12:03:04 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision branch-2 / f4c91ee
Default Java AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count 60 (vs. ulimit of 12500)
modules C: hbase-server U: hbase-server
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4173/1/console
versions git=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@stoty
Copy link
Contributor

stoty commented Mar 7, 2022

Interesting. For branch-2.4 we're compiling phoenix with Hbase 2.4.1, so we don't see the difference at compile time.
Our Scheduler code itself doesn't throw exceptions on dispatch(), any exception may only come from the delegate scheduler, which is coming the HBase code, so in theory we shouldn't even notice even if the API changes in Hbase 2.4.

For 2.5 and later, we're going to have to rename our internal dispatch() method, and add a dispatch() wrapper to the compatibility shim that handles the API differences betweeen the HBase versions, instead of overriding it as we do now.

All in all, Phoenix can handle the API change in HBase 2.5+ .

@ndimiduk
Copy link
Member Author

ndimiduk commented Mar 7, 2022

All in all, Phoenix can handle the API change in HBase 2.5+

Is that for just this change to RpcExecutor, or are you looking at changes to RpcScheduler#dispatch ?

Thanks @stoty .

@stoty
Copy link
Contributor

stoty commented Mar 7, 2022

I've only looked at RPCExecutor#dispatch() .

@stoty
Copy link
Contributor

stoty commented Mar 7, 2022

But RpcScheduler#dispatch can be handled the same way.

@ndimiduk
Copy link
Member Author

ndimiduk commented Mar 8, 2022

I wouldn't want to make any interface compatibility changes to IA.LimitedPrivate classes on a patch release, only minor release. Thank again @stoty for taking the time!

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 2m 27s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ branch-2 Compile Tests _
+1 💚 mvninstall 3m 39s branch-2 passed
+1 💚 compile 2m 51s branch-2 passed
+1 💚 checkstyle 0m 44s branch-2 passed
+1 💚 spotbugs 1m 35s branch-2 passed
_ Patch Compile Tests _
+1 💚 mvninstall 3m 9s the patch passed
+1 💚 compile 2m 57s the patch passed
+1 💚 javac 2m 57s the patch passed
+1 💚 checkstyle 0m 45s hbase-server: The patch generated 0 new + 1 unchanged - 14 fixed = 1 total (was 15)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 hadoopcheck 9m 18s Patch does not cause any errors with Hadoop 3.1.2 3.2.1.
+1 💚 spotbugs 1m 57s the patch passed
_ Other Tests _
+1 💚 asflicense 0m 11s The patch does not generate ASF License warnings.
35m 30s
Subsystem Report/Notes
Docker ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4173/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #4173
Optional Tests dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
uname Linux 76305228b76a 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision branch-2 / 5bae04e
Default Java AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count 60 (vs. ulimit of 12500)
modules C: hbase-server U: hbase-server
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4173/2/console
versions git=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 2m 18s Docker mode activated.
-0 ⚠️ yetus 0m 5s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ branch-2 Compile Tests _
+1 💚 mvninstall 4m 11s branch-2 passed
+1 💚 compile 0m 53s branch-2 passed
+1 💚 shadedjars 4m 19s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 43s branch-2 passed
_ Patch Compile Tests _
+1 💚 mvninstall 3m 37s the patch passed
+1 💚 compile 0m 55s the patch passed
+1 💚 javac 0m 55s the patch passed
+1 💚 shadedjars 4m 9s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 27s the patch passed
_ Other Tests _
+1 💚 unit 363m 27s hbase-server in the patch passed.
386m 21s
Subsystem Report/Notes
Docker ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4173/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR #4173
Optional Tests javac javadoc unit shadedjars compile
uname Linux 1846cd652ed9 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision branch-2 / 5bae04e
Default Java AdoptOpenJDK-11.0.10+9
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4173/2/testReport/
Max. process+thread count 1818 (vs. ulimit of 12500)
modules C: hbase-server U: hbase-server
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4173/2/console
versions git=2.17.1 maven=3.6.3
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@ndimiduk ndimiduk merged commit 7e267c6 into apache:branch-2 Mar 9, 2022
@ndimiduk ndimiduk deleted the 26782-cleanup-rpcexecutor-branch-2 branch March 9, 2022 11:44
@ndimiduk
Copy link
Member Author

ndimiduk commented Mar 9, 2022

Okay, so here's a problem that confused me through this. RpcExecutor, the abstract base class, is marked as IA.Private. However, it has several subclasses that are marked as IA.LimitedPrivate(COPROC, PHOENIX).

https://issues.apache.org/jira/browse/HBASE-26817

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport This PR is a back port of some issue or issues already committed to master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants