HBASE-23957 [flakey test] client.TestMultiParallel fails to read hbas… #1310
Conversation
This file contains 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
saintstack
reviewed
Mar 18, 2020
| private final Path cnfPath = FileSystems.getDefault().getPath("target/test-classes/hbase-site.xml"); | ||
| private final Path cnf2Path = FileSystems.getDefault().getPath("target/test-classes/hbase-site2.xml"); | ||
| private final Path cnf3Path = FileSystems.getDefault().getPath("target/test-classes/hbase-site3.xml"); | ||
| private static Path newCnfPath, newCnf2Path, newCnf3Path; |
There was a problem hiding this comment.
Does it have to be static? Can these be done in @before per test?
There was a problem hiding this comment.
There are shared across all test cases and just need to be initialized once.
|
|
||
| String absoluteDataPath = TEST_UTIL.getDataTestDir().toString(); | ||
| String dataBasePath = System.getProperty(HBaseCommonTestingUtility.BASE_TEST_DIRECTORY_KEY, | ||
| HBaseCommonTestingUtility.DEFAULT_BASE_TEST_DIRECTORY); |
There was a problem hiding this comment.
This is interesting. Do you not trust what you get from TEST_UTIL#getDataTestDir?
There was a problem hiding this comment.
The one from getDataTestDir is the absolute path. It needs to get the relative path here so need to have base directory. Maybe it should not expose such details from internal, let me wrap up an API from TEST_UTIL.
There was a problem hiding this comment.
Hi @saintstack, I made HBaseCommonTestingUtility#getBaseTestDir public as I need this API to avoid hack the details of how base test dir is created in the testing code.
| String dataPath = absoluteDataPath.substring(absoluteDataPath.indexOf(dataBasePath)); | ||
| newCnfPath = Paths.get(dataPath + "/hbase-site.xml"); | ||
| newCnf2Path = Paths.get(dataPath + "/hbase-site2.xml"); | ||
| newCnf3Path = Paths.get(dataPath + "/hbase-site3.xml"); |
There was a problem hiding this comment.
This is the dir above the test dir?
There was a problem hiding this comment.
Is this inside the test dir?
There was a problem hiding this comment.
Yeah, now the mutable hbase-site.xml goes to test-data dir instead of test-classes.
There was a problem hiding this comment.
Maybe in comment give example so can see better what is going in here.
There was a problem hiding this comment.
Will add more comments to explain why these changes are needed, thanks!
There was a problem hiding this comment.
Comments are added to explain how it works and what is the cause for this change.
| Files.createDirectories(Paths.get(dataPath)); | ||
| Files.copy(cnfPath, newCnfPath); | ||
| Files.copy(cnf2Path, newCnf2Path); | ||
| Files.copy(cnf3Path, newCnf3Path); |
There was a problem hiding this comment.
Does not seem have a good way to make these in a loop.
There was a problem hiding this comment.
Just have a new idea and will make it a loop to remove duplicates.
There was a problem hiding this comment.
Improve the code a bit to avoid duplicate code.
| Files.copy(cnf3Path, newCnf3Path); | ||
|
|
||
| // Add the new custom config file to Configuration | ||
| TEST_UTIL.getConfiguration().addResource(TEST_UTIL.getDataTestDir("hbase-site.xml")); |
There was a problem hiding this comment.
Did this get copied into the dir above? Which part?
There was a problem hiding this comment.
yeah, the above copies hbase-site.xml from test-classes to test-data, then the copy under test-data is added to config resources. When hbase-site.xml under test-data is updated, Configuration can be reloaded to include changes.
There was a problem hiding this comment.
So there are three resources for the config now, the default are
hbase-defaults.xml/hbase-site.xml under test-classes, which are immutable and shared across different testing cases. hbase-site.xml under test-data is per test case and only this file will be changed to avoid bumping into other tests.
| // again. | ||
| for (int i = 0; i < 2; i ++) { | ||
| TEST_UTIL.getMiniHBaseCluster().getRegionServer(i).getConfiguration(). | ||
| addResource(TEST_UTIL.getDataTestDir("hbase-site.xml")); |
There was a problem hiding this comment.
Interesting. Can we fix the base problem? Good find though.
There was a problem hiding this comment.
The issue is like that, the following code tries to clone a copy of conf for region servers. However, it is not really a clone, it just copies attributes over and other infos such as resources are lost which is important to test the updated config for region servers. Check the history of why cloning Configuration is not used (the clone one was used before and got reverted). Adding another method in HBaseConfiguration for this testing purpose seems an overkill and causes confusion. Hence the hack in testing code instead of making changes in common code.
There was a problem hiding this comment.
oh... i remember that. Ok. Add comment here to explain why you have to do this. Good stuff.
| Files.createDirectories(Paths.get(dataPath)); | ||
| Files.copy(cnfPath, newCnfPath); | ||
| Files.copy(cnf2Path, newCnf2Path); | ||
| Files.copy(cnf3Path, newCnf3Path); |
There was a problem hiding this comment.
Duped code. Any way of sharing ?
There was a problem hiding this comment.
Thought about it, let me come back to see if there is good way to share code between these two testing classes.
There was a problem hiding this comment.
Will leave it as it is now, do not look that bad after changing to loop, :)
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
qiaoandxiang
force-pushed
the
master.HBASE-23957
branch
from
d638990
to
59e4fa1
Compare
|
💔 -1 overall
This message was automatically generated. |
saintstack
approved these changes
Mar 19, 2020
| // Before this change, the test will update hbase-site.xml under target/test-classes and | ||
| // trigger a config reload. Since target/test-classes/hbase-site.xml is being used by | ||
| // other testing cases at the same time, this update will break other testing cases so it will | ||
| // be flakey in nature. |
| // hbase-site.xml will be created under its test data directory, i.e, | ||
| // hbase-server/target/test-data/UUID, this new file will be added as a resource for the | ||
| // config, new update will be applied to this new file and only visible to this specific test | ||
| // case. The target/test-classes/hbase-site.xml will not be changed during the test. |
|
💔 -1 overall
This message was automatically generated. |
ndimiduk
requested changes
Mar 19, 2020
| @@ -207,7 +207,7 @@ boolean cleanupTestDir(final String subdir) { | |||
| * Unit test will use a subdirectory of this directory. | |||
| * @see #setupDataTestDir() | |||
| */ | |||
| private Path getBaseTestDir() { | |||
| public Path getBaseTestDir() { | |||
There was a problem hiding this comment.
You sure you should change the privacy here? The javadoc comment here literally says "Should not be used by the unit tests, hence its's private." Seems like callers are supposed to use getDataTestDir
There was a problem hiding this comment.
I will go back to check if getDataTestDir is enough for this test.
There was a problem hiding this comment.
I debugged more and found that indeed this change is not needed.
| // Before this change, the test will update hbase-site.xml under target/test-classes and | ||
| // trigger a config reload. Since target/test-classes/hbase-site.xml is being used by | ||
| // other testing cases at the same time, this update will break other testing cases so it will | ||
| // be flakey in nature. |
| // case. The target/test-classes/hbase-site.xml will not be changed during the test. | ||
| String absoluteDataPath = TEST_UTIL.getDataTestDir().toString(); | ||
| String dataBasePath = TEST_UTIL.getBaseTestDir().toString(); | ||
| String dataPath = absoluteDataPath.substring(absoluteDataPath.indexOf(dataBasePath)); |
There was a problem hiding this comment.
Yikes! Can you use the File or Path APIs instead of doing string manipulation?
Why is a relative path necessary anyway?
There was a problem hiding this comment.
I actually tried to use absolute path at first and run into exceptions. I did not dig further and backed to the relative path.
Let me debug a bit more here and then getBaseTestDir can keep private.
There was a problem hiding this comment.
Yeah, just debugged and found this change is not needed. Will upload a new patch.
|
|
||
| List<String> confs = Arrays.asList("/hbase-site.xml", "/hbase-site2.xml", "/hbase-site3.xml"); | ||
| for (String conf : confs) { | ||
| Path cnfPath = FileSystems.getDefault().getPath("target/test-classes" + conf); |
There was a problem hiding this comment.
Please use the API of Path to build these. For example, Path(Path parent, String child) lets you create an instance from a base path plus a string for the child component.
There was a problem hiding this comment.
Let me change that.
There was a problem hiding this comment.
Path here is the Jvm path, not the hadoop path, changed it to Paths.get() to be more maintainable.
|
|
||
| // The resources added to TEST_UTIL.conf is lost during Region Server Creation, hack it here | ||
| // again. | ||
| for (int i = 0; i < 2; i ++) { |
There was a problem hiding this comment.
Two questions:
- can this test get away with starting only one region server instead of 2?
- can you use a non-indexed for-loop over
MiniHBaseCluster$getRegionServerThreads()instead of an indexed for-loop?
There was a problem hiding this comment.
Hmm, I think it is possible to use 1 region server. The original test uses 2 region servers so I kept that way.
For 2, yeah, definitely possible and I will make change in my next patch.
| @@ -139,14 +173,14 @@ public void testAllClusterOnlineConfigChange() throws IOException { | |||
|
|
|||
| private void replaceHBaseSiteXML() throws IOException { | |||
| // make a backup of hbase-site.xml | |||
| Files.copy(cnfPath, cnf3Path, StandardCopyOption.REPLACE_EXISTING); | |||
| Files.copy(newCnfPathes.get(0), newCnfPathes.get(2), StandardCopyOption.REPLACE_EXISTING); | |||
There was a problem hiding this comment.
nit for other readers: since you're here cleaning up, can you use descriptive names for these variables? Instead of cnfPath{1,2,3} or this array of paths, how about
rsConfigFilePathinstead ofcnfPathoverwriteFilePathinstead ofcnf1PathrsConfigFileBackupinstead ofcnf3Path
IMHO, descriptive variable names are better than comments. Just a suggestion :)
There was a problem hiding this comment.
Sure, will do, :)
| // hbase-server/target/test-data/UUID, this new file will be added as a resource for the | ||
| // config, new update will be applied to this new file and only visible to this specific test | ||
| // case. The target/test-classes/hbase-site.xml will not be changed during the test. | ||
| String absoluteDataPath = TEST_UTIL.getDataTestDir().toString(); |
There was a problem hiding this comment.
same comments about using File or Path api instead of string manipulation.
|
|
||
| // The resources added to TEST_UTIL.conf is lost during Region Server Creation, hack it here | ||
| // again. | ||
| TEST_UTIL.getMiniHBaseCluster().getRegionServer(0).getConfiguration(). |
There was a problem hiding this comment.
same comment about using iteration over all cluster members instead of hard-coding an assumed number.
| } | ||
|
|
||
| @Test | ||
| public void testOnlineConfigChange() throws IOException { | ||
| LOG.debug("Starting the test"); | ||
| LOG.debug("Starting the test testOnlineConfigChange"); |
There was a problem hiding this comment.
for places where you want to refer to the test's method name, JUnit provides an API for this. Check out org.junit.rules.TestName. It's used throughout the code base.
There was a problem hiding this comment.
Thanks for bringing up my old memory, I know there is a way to avoid hardcoded method name, just forgot, :)
There was a problem hiding this comment.
Hi @ndimiduk, I updated the patch. Hopefully I did not miss some. There are duplicate codes between these two testing cases. I thought about it and did not find a good way to avoid it. Any suggestions are welcome, thanks.
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestUpdateConfiguration.java
Show resolved
Hide resolved
qiaoandxiang
force-pushed
the
master.HBASE-23957
branch
from
59e4fa1
to
72fa760
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
I found the error, it is due to that I did not do a mvn clean first so the error was not caught at my local run. Working on an fix and an abstract class to share the common logic for these two testing classes. |
qiaoandxiang
force-pushed
the
master.HBASE-23957
branch
from
72fa760
to
2200d1a
Compare
|
Updated the patch and let's see what testbot says. |
|
💔 -1 overall
This message was automatically generated. |
|
Will fix the checkstyle warning soon. |
|
💔 -1 overall
This message was automatically generated. |
qiaoandxiang
force-pushed
the
master.HBASE-23957
branch
from
2200d1a
to
e4d6677
Compare
|
Update for checkstyle warning. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
saintstack
approved these changes
Mar 20, 2020
hbase-server/src/test/java/org/apache/hadoop/hbase/client/AbstractTestUpdateConfiguration.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
qiaoandxiang
force-pushed
the
master.HBASE-23957
branch
from
e4d6677
to
f80dd68
Compare
ndimiduk
approved these changes
Mar 20, 2020
hbase-server/src/test/java/org/apache/hadoop/hbase/client/AbstractTestUpdateConfiguration.java
Show resolved
Hide resolved
| * Base class to test Configuration Update logic. | ||
| */ | ||
| public abstract class AbstractTestUpdateConfiguration { | ||
| private static final int SERVER_CONFIG_INDEX = 0; |
There was a problem hiding this comment.
Why track indices when you could just have three variables like
private static final String SERVER_CONFIG = "hbase-site.xml";
private static final String OVERWRITE_SERVER_CONFIG = "hbase-site2.xml";
...This is what I meant in my earlier comment about descriptive variable names.
And for easier debugging, why call the file names 1, 2, 3 -- they can be named whatever we want since we're manually adding them as resources. Might as well make the file names descriptive too, so someone doing forensics later can have a clue. Something like "copy-of-hbase-site.xml", "overrides-for-test-hbase-site.xml", &c.
There was a problem hiding this comment.
IMHO, the tests as they were before you started were illegible and obtuse. I'm arguing that they should be easily understood by casual observation, not deep study. One way to achieve that is by well-considered, meaningful variable names and simple logic without lots of flow control.
There was a problem hiding this comment.
Agreed, was trying to avoid duplicate codes, let me update and upload a new patch.
|
|
||
| List<String> confs = Arrays.asList("/hbase-site.xml", "/hbase-site2.xml", "/hbase-site3.xml"); | ||
| for (String conf : confs) { | ||
| Path cnfPath = Paths.get("target/test-classes" + conf); |
There was a problem hiding this comment.
Again you're concatenating strings instead of using the API as provided. How about
final Path confPath = Paths.get("target", "test-classes", "hbase-site.xml");
hbase-server/src/test/java/org/apache/hadoop/hbase/client/AbstractTestUpdateConfiguration.java
Show resolved
Hide resolved
| Path cnfPath = Paths.get("target/test-classes" + conf); | ||
| Path newConfPath = Paths.get(absoluteDataPath + conf); | ||
|
|
||
| // Do not copy the last one as it does not exist |
There was a problem hiding this comment.
i'm surprised an hbase-site2.xml exists.
There was a problem hiding this comment.
It is actually named as backup-hbase-site.xml now, :)
|
Left a comment in wrong issue. |
|
💔 -1 overall
This message was automatically generated. |
qiaoandxiang
force-pushed
the
master.HBASE-23957
branch
from
0ed5ab9
to
84cba8e
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
qiaoandxiang
force-pushed
the
master.HBASE-23957
branch
from
84cba8e
to
8c54939
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Good to go I'd say @qiaoandxiang . Merge it here by hitting above 'squash and merge'. This will take you to a new textbox where you can adjust the commit message. Add in this 'Signed-off-by: Nick Dimiduk ndimiduk@apache.org' to show it was signed off by Nick. Then hit merge. Then there are various options for getting it to other branches. I manually cherry-pick usually. Cherry-pick to branch-2, branch-2.3, and branch-2.2 at least. See if it will go to branch-1 too since that one is active and this an old issue. Good stuff. |
|
Thanks Stack, will do it later today. Need to set up my committer account first.
Get Outlook for Android<https://aka.ms/ghei36>
…________________________________
From: Michael Stack <notifications@github.com>
Sent: Saturday, March 21, 2020 12:15:40 PM
To: apache/hbase <hbase@noreply.github.com>
Cc: Huaxiang Sun <qiaoandxiang@hotmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [apache/hbase] HBASE-23957 [flakey test] client.TestMultiParallel fails to read hbas… (#1310)
Good to go I'd say @qiaoandxiang<https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fqiaoandxiang&data=02%7C01%7C%7Cfb40ffdcbe4e4b8e6eb308d7cdcc3f51%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637204149432275461&sdata=hliE77vE%2FTKoPRNGzDlUOdBeTeMXpU1IF3CXZN%2BTpRc%3D&reserved=0> . Merge it here by hitting above 'squash and merge'. This will take you to a new textbox where you can adjust the commit message. Add in this 'Signed-off-by: Nick Dimiduk ndimiduk@apache.org<mailto:ndimiduk@apache.org>' to show it was signed off by Nick. Then hit merge.
Then there are various options for getting it to other branches. I manually cherry-pick usually. Cherry-pick to branch-2, branch-2.3, and branch-2.2 at least. See if it will go to branch-1 too since that one is active and this an old issue. Good stuff.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fhbase%2Fpull%2F1310%23issuecomment-602089802&data=02%7C01%7C%7Cfb40ffdcbe4e4b8e6eb308d7cdcc3f51%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637204149432275461&sdata=vQHL548LOOaB6had1bFWqTda%2F%2BrwVRZVjg%2FxeS2%2BBv8%3D&reserved=0>, or unsubscribe<https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFB6WBY27MTOJOHQFYLRK5LRIUG5ZANCNFSM4LO4TC6Q&data=02%7C01%7C%7Cfb40ffdcbe4e4b8e6eb308d7cdcc3f51%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637204149432285466&sdata=VPrTJg0LX1%2B40Manf1Y1BQu8MSSQe3OrOlUwYWwnEKg%3D&reserved=0>.
|
qiaoandxiang
added a commit
to qiaoandxiang/hbase-1
that referenced
this pull request
Mar 23, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk ndimiduk@apache.org Signed-off-by: stack <stack@apache.org>
qiaoandxiang
added a commit
to qiaoandxiang/hbase-1
that referenced
this pull request
Mar 23, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk ndimiduk@apache.org Signed-off-by: stack <stack@apache.org>
qiaoandxiang
added a commit
to qiaoandxiang/hbase-1
that referenced
this pull request
Mar 23, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk ndimiduk@apache.org Signed-off-by: stack <stack@apache.org>
qiaoandxiang
added a commit
to qiaoandxiang/hbase-1
that referenced
this pull request
Mar 23, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: stack <stack@apache.org>
qiaoandxiang
added a commit
to qiaoandxiang/hbase-1
that referenced
this pull request
Mar 25, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: stack <stack@apache.org>
qiaoandxiang
added a commit
to qiaoandxiang/hbase-1
that referenced
this pull request
Mar 25, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: stack <stack@apache.org>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk ndimiduk@apache.org Signed-off-by: stack <stack@apache.org>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
…e-site.xml (apache#1310) Signed-off-by: Nick Dimiduk ndimiduk@apache.org Signed-off-by: stack <stack@apache.org>
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
…e-site.xml (apache#1310) (apache#1329) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: stack <stack@apache.org> (cherry picked from commit 33143f2) Change-Id: Ife90722dcad256c36b0d2697d20ecf0d3eff3fcb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…e-site.xml
Use a customized hbase-site.xml for updating configuration, leave hbase-site.xml immutable under target/test-classes so it wont interrupt other parallel tests.