[
https://issues.apache.org/jira/browse/HBASE-20444?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16475782#comment-16475782
]
Sean Busbey commented on HBASE-20444:
-------------------------------------
{quote}
3.commons-lang's StringUtils#isNumeric cannot pass checkstyle due to
checkstyle.xml
{code}
<module name="IllegalImport">
<property name="illegalPkgs" value="
com.google.common,
io.netty,
org.apache.commons.cli,
org.apache.commons.collections,
org.apache.commons.collections4,
org.apache.commons.lang,
org.apache.curator.shaded,
org.apache.htrace.shaded"/>
</module>
{code}
{quote}
We should probably give pointers here for what folks ought to use. The
commons-lang check here is trying to make sure folks don't use commons-lang
2.z. [the commons-lang 3 version of
StringUtils.isNumeric|https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#isNumeric-java.lang.CharSequence-]
should pass checkstyle.
That said, I don't think everything that {{StringUtils}} identifies as numeric
will pass muster with {{Integer.parseInt}}.
{code}
171 private static boolean isNumeric(String str) {
172 Pattern pattern = Pattern.compile("[0-9]*");
173 return pattern.matcher(str).matches();
174 }
{code}
Negative numbers get treated as strings for sort, is that intentional? What
happens when we pass in an integer > MAX_INT?
Can we include some erroneous version strings in the test set to make sure we
have fewer surprises in the future? like "foobar-1.2.3".
Also a comparison within alpha/beta to confirm numeric is being used amongst
them. e.g. "3.0.0-alpha-2" vs "3.0.0-alpha-11"
{code}
42 assertTrue(VersionInfo.compareVersion("2.0.0", "2.0.0.0") == 0);
{code}
I don't think this is correct. "critical patch 0" should be after the main
release. I'm not sure we've ever done a critical patch release, but still.
{code}
52 assertTrue(VersionInfo.compareVersion("2.0.0.1", "2.0.0-fuck") < 0);
53 assertTrue(VersionInfo.compareVersion("2.0.0-ooxx", "2.0.0.1") > 0);
{code}
No swears please. something like "2.0.0-foobar" would suffice for "there's a
label string we don't know about". Also, I think the ordering here is wrong as
well "2.0.0.1" would be "the second critical patch release on 2.0.0" should
occur after "2.0.0 with some caveat we don't recognize".
> Improve version comparison logic for HBase specific version string and add
> unit tests
> -------------------------------------------------------------------------------------
>
> Key: HBASE-20444
> URL: https://issues.apache.org/jira/browse/HBASE-20444
> Project: HBase
> Issue Type: Improvement
> Reporter: Umesh Agashe
> Assignee: maoling
> Priority: Major
> Attachments: HBASE-20444.master.001.patch,
> HBASE-20444.master.002.patch, HBASE-20444.master.003.patch
>
>
> As [~busbey] commented on HBASE-18792, current logic for comparing version
> strings in class org.apache.hadoop.hbase.util.VersionInfo is generic and
> needs to be improved:
> {code:java}
> if (index < s1.length) {
> // s1 is longer
> return 1;
> }
> {code}
> {quote}I think this is wrong? like version "2.0.0" should be after
> "2.0.0-SNAPSHOT". it's also after "2.0.0-alpha-3" or "2.0.0-beta-1".
> {quote}
> Also in other cases 2.0.0 should be before 2.0.0-patch-XXXX and 2.0.0.1. Also
> 2.0 should be before 2.0.1.
> {quote}Can we expand the versions checked in TestVersionInfo to include a)
> some "same major different minor", b) "same minor different maintenance", c)
> both of the above, but SNAPSHOT, d) "-alpha" / "-beta"?
> {quote}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
