Hbase 26167 #3849
Closed
Hbase 26167 #3849
Conversation
This file contains hidden or 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
merge apache/hbase
…per and dfs cluster.
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Thank you for taking this up, mind reply on the jira issue so I can assign the issue to you? And sorry the approach is not correct, HBTU itself already has the ability to not start zk, you just need to call setZkCluster to inject the external zk cluster. The goal for HBASE-26167 is to make TestingHBaseCluster can use external zk cluster, for maximum flexibility, I prefer we just add a parameter in the TestingHBaseClusterOption to let user specify the connectString for zookeeper, and once this parameter is set, we will not start zk cluster by ourselves but just initialize the zk client with the given connectString. WDYT? Thanks. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
Any updates here? Thanks. |
…e TestingHBaseClusterImpl to use enternal dfs or zk
Contributor
Author
|
I'm sorry for the delay in updating. Please review. If there is anything unreasonable, let me fix. |
|
💔 -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. |
Contributor
Author
|
I run the test of TestDisableTableProcedure successfully on my computer, but the jenkins gives a TestTimedOutException, how can I solve the problem? |
Contributor
|
You should check the newest PR build. https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3849/2/testReport/ The failed UTs are TestMemStoreSegmentsIterator and TestRecoveredEditsReplayAndAbort. |
Contributor
Author
|
The last commit is to fix failure of TestMemStoreSegmentsIterator and TestRecoveredEditsReplayAndAbort. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Dec 18, 2021
| import org.apache.hadoop.hbase.zookeeper.EmptyWatcher; | ||
| import org.apache.hadoop.hbase.zookeeper.ZKConfig; | ||
| import org.apache.hadoop.hbase.zookeeper.ZKWatcher; | ||
| import org.apache.hadoop.hbase.zookeeper.*; |
|
|
||
| import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; | ||
|
|
||
| import static org.junit.Assert.*; |
Contributor
There was a problem hiding this comment.
Ditto. And static imports should be place on top.
| this.conf.set("fs.defaultFS", "file:///"); | ||
| this.conf.set(HConstants.HBASE_DIR, "file://" + dataTestDir); | ||
| LOG.debug("Setting {} to {}", HConstants.HBASE_DIR, dataTestDir); | ||
| if (this.conf.get("fs.defaultFS") != null && !this.conf.get("fs.defaultFS").startsWith("hdfs://")) { |
Contributor
There was a problem hiding this comment.
I guess here you do not want to override the fs.defaultFS config if it is present? But the logic here will skip setting fs.defaultFS if it is not set? Is this correct?
| throws IOException, InterruptedException { | ||
|
|
||
| if (option.isExternalDFS()) { | ||
| assertNotNull("fs.defaultFS can not be null, if use external DFS", conf.get("fs.defaultFS")); |
Contributor
There was a problem hiding this comment.
In general I prefer we do not depend on junit methods in HBTU, as it will force our downstream users also use the same version of junit... But anyway, not your fault, I think we have already used it in this class...
| * If zk or hdfs is external, clean the znode or dfs path. | ||
| * @see #startMiniCluster(int) | ||
| */ | ||
| public void shutdownMiniCluster(StartTestingClusterOption option) throws IOException { |
Contributor
There was a problem hiding this comment.
I think we should know whether we have started DFS cluster and zk by our own? Or at least we can store the StartTestingClusterOption so users do not need to pass it again?
| * If use external dfs or zk, clean the directory. | ||
| * @throws java.io.IOException in case command is unsuccessful | ||
| */ | ||
| public void shutdownMiniHBaseCluster(StartTestingClusterOption option) throws IOException { |
| return createWALDir; | ||
| } | ||
|
|
||
| public boolean isExternalDFS() { |
Contributor
There was a problem hiding this comment.
I prefer here we pass in the necessary configs to connecting an external HDFS, and set them into the Configuration object.
Contributor
Author
There was a problem hiding this comment.
It would be complicated to pass all the required configuration in kv mode, especially HA or Kerberos, I want to pass an HDFS conf and add it to this.conf, is that ok?
| return externalDFS; | ||
| } | ||
|
|
||
| public boolean isExternalZK() { |
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
I have a problem, I run all the unit test with "mvn clean test" several times, The failed UTs are different each time. Then I run the failed UT with "mvn clean test -Dtest=xxx", it is successful. The same thing happens when I switch to the Master branch. |
|
🎊 +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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.