-
Notifications
You must be signed in to change notification settings - Fork 150
HBASE-27119 [HBCK2] Some commands are broken after HBASE-24587 #107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Change-Id: Idcd4096f02db3f922d086252b573e64336fd45f1
| Command: | ||
| addFsRegionsMissingInMeta <NAMESPACE|NAMESPACE:TABLENAME>...|-i <INPUT_FILE>... | ||
| Options: | ||
| -d,--force_disable aborts fix for table if disable fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed this option was not being handled in the actual command, so removed it from readme and command helper below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
| <artifactId>log4j-api</artifactId> | ||
| <version>${log4j2.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst testing against a pseudo-distributed 2.6.0-snapshot deployment, was facing conflicts with the slf4j classes shipped in hbck2 jar and the ones loaded from hbase client lib, so marked it as provided to avoid shipping those iin the hbck2 jar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will hbck2 be used as a standalone command line tool? If not, then I do not think we need to include these jars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could, but we normally run it using the hbase script, so that we get CP built, in which case, shipping slf4j classes within the hbck2 jar was causing below problems to me:
SLF4J: Class path contains multiple SLF4J bindings. SLF4J: Found binding in [jar:file:/root/hbase-3.0.0-alpha-3-SNAPSHOT/hbase-hbck2-1.3.0-SNAPSHOT.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: Found binding in [jar:file:/root/hbase-3.0.0-alpha-3-SNAPSHOT/lib/client-facing-thirdparty/log4j-slf4j-impl-2.17.2.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation. SLF4J: Actual binding is of type [org.apache.logging.slf4j.Log4jLoggerFactory] Exception in thread "main" java.lang.ExceptionInInitializerError Caused by: java.lang.StringIndexOutOfBoundsException: String index out of range: -1 at java.lang.String.substring(String.java:1967) at org.apache.logging.log4j.util.PropertiesUtil.partitionOnCommonPrefixes(PropertiesUtil.java:555) at org.apache.logging.log4j.core.config.properties.PropertiesConfigurationBuilder.build(PropertiesConfigurationBuilder.java:174) at org.apache.logging.log4j.core.config.properties.PropertiesConfigurationFactory.getConfiguration(PropertiesConfigurationFactory.java:56) at org.apache.logging.log4j.core.config.properties.PropertiesConfigurationFactory.getConfiguration(PropertiesConfigurationFactory.java:35) at org.apache.logging.log4j.core.config.ConfigurationFactory$Factory.getConfiguration(ConfigurationFactory.java:557) at org.apache.logging.log4j.core.config.ConfigurationFactory$Factory.getConfiguration(ConfigurationFactory.java:481) at org.apache.logging.log4j.core.config.ConfigurationFactory.getConfiguration(ConfigurationFactory.java:323) at org.apache.logging.log4j.core.LoggerContext.reconfigure(LoggerContext.java:695) at org.apache.logging.log4j.core.LoggerContext.reconfigure(LoggerContext.java:716) at org.apache.logging.log4j.core.LoggerContext.start(LoggerContext.java:270) at org.apache.logging.log4j.core.impl.Log4jContextFactory.getContext(Log4jContextFactory.java:155) at org.apache.logging.log4j.core.impl.Log4jContextFactory.getContext(Log4jContextFactory.java:47) at org.apache.logging.log4j.LogManager.getContext(LogManager.java:196) at org.apache.logging.log4j.spi.AbstractLoggerAdapter.getContext(AbstractLoggerAdapter.java:137) at org.apache.logging.slf4j.Log4jLoggerFactory.getContext(Log4jLoggerFactory.java:55) at org.apache.logging.log4j.spi.AbstractLoggerAdapter.getLogger(AbstractLoggerAdapter.java:47) at org.apache.logging.slf4j.Log4jLoggerFactory.getLogger(Log4jLoggerFactory.java:33) at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:358) at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:383) at org.apache.hbase.HBCK2.<clinit>(HBCK2.java:91)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means we should exclude the log4j related classes in the shaded jar. If we will run it as a standalone command, we should ship the log4j related jars in the final tarball.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe this is a broader issue and we should address in a separate jira, as I noticed the following classpath issues whilst testing it against a pseudo-distributed 2.6.0-SNASPHOT:
- If I try run hbck2 as ./bin/hbase hbck -j ../hbase-hbck2-1.3.0-SNAPSHOT.jar, I get below error:
Error: Could not find or load main class org.apache.hbase.HBCK2 Caused by: java.lang.NoClassDefFoundError: org/apache/hadoop/util/Tool
- If I try run the hbck2 jar that includes slf4j classes as ./bin/hbase --internal-classpath hbck -j ../hbase-hbck2-1.3.0-SNAPSHOT.jar, it passes the NCDFE mentioned above, but it then gets the error from my previous comment. So maybe this slf4j conflict is due to this
--internal-classpathflag that shouldn't be used. e
|
💔 -1 overall
This message was automatically generated. |
Apache9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind explaining a bit on what is the problem here? After skimming the PR, I can not fully understand what is the problem we are trying to fix here...
| <artifactId>log4j-api</artifactId> | ||
| <version>${log4j2.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will hbck2 be used as a standalone command line tool? If not, then I do not think we need to include these jars.
HBCK2 replication and filesystem commands are broken after HBASE-24587. Trying to pass the f or -fix options give the below error: This is because getInputList calls here and here only accept the -i/--inputFiles options, throwing an exception if we pass -f/--fix options. |
|
FYI @clarax |
Apache9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I can partly know what is the problem now, but the whole implementation seems a mess to me...
Why don't create an Options for each command separately? And then parse it? Because most command share almost the same pattern?
Change-Id: Ic8156b9f0c2ecefc3abc3dacdf0829e7a23f58a0
I'm happy to answer any questions you might still have. If you feel confused because of the extra pom, readme and command help changes which are not related to the issue's title here, I can revert those and put in extra PR's.
Originally thought about that, but then decided better to do the parsing once. |
Change-Id: I772e05f6f283f939d3d0ba3de2e13cbdc6bb3910
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks a lot for adding the comprehensive UT. Please see an alternative fix that follows the same pattern for other commands with command options. #109 |
|
LGTM. See conversation at #109. |
Change-Id: Ibed1b7b741216082e8b6d542ccaa5f595af40dd9
|
🎊 +1 overall
This message was automatically generated. |
hbase-hbck2/src/test/java/org/apache/hbase/TestHBCKCommandLineParsing.java
Outdated
Show resolved
Hide resolved
hbase-hbck2/src/test/java/org/apache/hbase/TestHBCKCommandLineParsing.java
Outdated
Show resolved
Hide resolved
Change-Id: Ic233deec2307474549f111b5b881dca7454c6256
Change-Id: I5795a4924c83576cf7a2bb70cc760470d0ff01d0
|
Thanks for the review, @petersomogyi . Had applied your suggestions. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
petersomogyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wchevreuil! Please fix the checkstyle warning before committing.
Change-Id: If88d15e3940db754eb178070be8444d5caf677cd
|
🎊 +1 overall
This message was automatically generated. |
No description provided.