Skip to content

Conversation

@Apache9
Copy link
Contributor

@Apache9 Apache9 commented Oct 27, 2025

…/queue storage

@Apache9 Apache9 self-assigned this Oct 27, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors replication storage initialization to support different HBase versions by centralizing version-specific reflection logic into a new helper class.

  • Removed version-specific reflection code from ReplicationChecker and moved it to a dedicated ReplicationStorageFactoryHelper class
  • Updated ReplicationChecker constructor to accept FileSystem and Connection parameters
  • Replaced direct ReplicationStorageFactory calls with the new helper throughout the codebase

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
ReplicationStorageFactoryHelper.java New helper class that encapsulates version-specific reflection logic for creating replication storage instances
ReplicationChecker.java Simplified constructor by removing reflection logic and delegating to the new helper class
HBaseFsck.java Updated calls to use the new helper class and pass additional parameters to ReplicationChecker

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 31s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 spotbugs 0m 0s spotbugs executables are not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-0 ⚠️ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+1 💚 mvninstall 1m 28s master passed
+1 💚 compile 0m 13s master passed
+1 💚 checkstyle 0m 12s master passed
+1 💚 spotless 0m 10s branch has no errors when running spotless:check.
+1 💚 javadoc 0m 10s master passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 15s the patch passed
+1 💚 compile 0m 13s the patch passed
+1 💚 javac 0m 13s the patch passed
-1 ❌ checkstyle 0m 6s hbase-hbck2: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 spotless 0m 6s patch has no errors when running spotless:check.
+1 💚 javadoc 0m 7s the patch passed
_ Other Tests _
+1 💚 unit 5m 27s hbase-hbck2 in the patch passed.
+1 💚 asflicense 0m 9s The patch does not generate ASF License warnings.
9m 20s
Subsystem Report/Notes
Docker ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-Operator-Tools-PreCommit/job/PR-156/1/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #156
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs spotless checkstyle compile
uname Linux 6e8861fca348 6.8.0-1024-aws #26~22.04.1-Ubuntu SMP Wed Feb 19 06:54:57 UTC 2025 x86_64 GNU/Linux
Build tool maven
Personality dev-support/jenkins/hbase-personality.sh
git revision master / fd37d47
Default Java Oracle Corporation-1.8.0_342-b07
checkstyle https://ci-hbase.apache.org/job/HBase-Operator-Tools-PreCommit/job/PR-156/1/artifact/yetus-precommit-check/output/diff-checkstyle-hbase-hbck2.txt
Test Results https://ci-hbase.apache.org/job/HBase-Operator-Tools-PreCommit/job/PR-156/1/testReport/
Max. process+thread count 1146 (vs. ulimit of 5000)
modules C: hbase-hbck2 U: hbase-hbck2
Console output https://ci-hbase.apache.org/job/HBase-Operator-Tools-PreCommit/job/PR-156/1/console
versions git=2.30.2 maven=3.8.6
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@petersomogyi
Copy link
Contributor

@Apache9 I still see compilation failures when building it with 3.0.0-beta-1 or 3.0.0-beta-2-SNAPSHOT.

[ERROR] /Users/psomogyi/projects/hbase-operator-tools/hbase-hbck2/src/main/java/org/apache/hbase/hbck1/ReplicationChecker.java:[73,46] cannot find symbol
[ERROR]   symbol:   method getListOfReplicators()
[ERROR]   location: variable queueStorage of type org.apache.hadoop.hbase.replication.ReplicationQueueStorage
[ERROR] /Users/psomogyi/projects/hbase-operator-tools/hbase-hbck2/src/main/java/org/apache/hbase/hbck1/ReplicationChecker.java:[74,41] cannot find symbol
[ERROR]   symbol:   method getAllQueues(org.apache.hadoop.hbase.ServerName)
[ERROR]   location: variable queueStorage of type org.apache.hadoop.hbase.replication.ReplicationQueueStorage
[ERROR] /Users/psomogyi/projects/hbase-operator-tools/hbase-hbck2/src/main/java/org/apache/hbase/hbck1/ReplicationChecker.java:[124,21] method removeQueue in interface org.apache.hadoop.hbase.replication.ReplicationQueueStorage cannot be applied to given types;
[ERROR]   required: org.apache.hadoop.hbase.replication.ReplicationQueueId
[ERROR]   found:    org.apache.hadoop.hbase.ServerName,java.lang.String
[ERROR]   reason: actual and formal argument lists differ in length
[ERROR] /Users/psomogyi/projects/hbase-operator-tools/hbase-hbck2/src/main/java/org/apache/hbase/hbck1/ReplicationChecker.java:[126,19] cannot find symbol
[ERROR]   symbol:   method removeReplicatorIfQueueIsEmpty(org.apache.hadoop.hbase.ServerName)
[ERROR]   location: variable queueStorage of type org.apache.hadoop.hbase.replication.ReplicationQueueStorage

@Apache9
Copy link
Contributor Author

Apache9 commented Oct 28, 2025

Ah, yes, the replication queue related methods are changed...

Let me think how to fix this. Seems on 3.0.0, we only need to call removeQueue(PeerId), the implementation of the method will scan all the replication queues for this peer and remove them all.

@Apache9
Copy link
Contributor Author

Apache9 commented Nov 1, 2025

For hbase 3.0.0, the layout has been changed. In the HBase table, peer id will be the first part, while on zookeeper, region server will be the first part.

So in this way, we do not need to scan all the region servers's directory to find out the undelete queues. So for 3.0.0, maybe we should add a listAllPeerIds method to ReplicationQueueStorage, and once there are peer ids which should be deleted, we call removeAllQueues(peerId) to cleanup these undeletedQueues.

@petersomogyi WDYT?

Thanks.

Sign up for free to 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