PHOENIX-5636: Improve the error message when client connects to server with higher major version #676

christinefeng · 2020-01-10T22:29:34Z

No description provided.

christinefeng · 2020-01-10T22:30:10Z

@ChinmaySKulkarni @dbwong @yanxinyi @abhishek-chouhan

sguggilam · 2020-01-31T02:33:00Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

        if (systemCatalogTimestamp < MIN_SYSTEM_TABLE_TIMESTAMP) {
            throw new UpgradeRequiredException(systemCatalogTimestamp);
        }
    }

+    private String displayClientandServerVersions(long serverJarVersion) {
+        String clientVersion = PHOENIX_MAJOR_VERSION + "." + PHOENIX_MINOR_VERSION + "." + PHOENIX_PATCH_NUMBER;


This is already present , you can probably just use it : https://github.com/apache/phoenix/blob/master/phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java#L151

sguggilam · 2020-01-31T02:33:51Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

-        if (isIncompatible) {
-            buf.setLength(buf.length()-1);
-            throw new SQLExceptionInfo.Builder(SQLExceptionCode.OUTDATED_JARS).setMessage(buf.toString()).build().buildException();
+        if (!(errorCode.toString().equals(new StringBuilder().toString()))) {


is this doing an empty string comparision ?

ChinmaySKulkarni

Overall looks good. I have some feedback, please take a look @christinefeng.

ChinmaySKulkarni · 2020-02-11T19:26:56Z

phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java

-        return VersionUtil.encodeMinPatchVersion(clientMajorVersion, clientMinorVersion) <= serverVersion && // Minor major and minor cannot be ahead of server
-                VersionUtil.encodeMaxMinorVersion(clientMajorVersion) >= serverVersion; // Major version must at least be up to server version
+        boolean isCompatible = true;
+        if (!(VersionUtil.encodeMinPatchVersion(clientMajorVersion, clientMinorVersion) <= serverVersion)) {


nit: simplify this condition to >

ChinmaySKulkarni · 2020-02-11T19:27:38Z

phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java

+        if (!(VersionUtil.encodeMinPatchVersion(clientMajorVersion, clientMinorVersion) <= serverVersion)) {
+            isCompatible = false;
+            buf.append(SQLExceptionCode.OUTDATED_JARS.getErrorCode());
+        } else if (!(VersionUtil.encodeMaxMinorVersion(clientMajorVersion) >= serverVersion)) {


same here. Change !>= to <

ChinmaySKulkarni · 2020-02-11T19:46:55Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

@@ -1487,20 +1487,17 @@ private static boolean hasIndexWALCodec(Long serverVersion) {
        return MetaDataUtil.decodeHasIndexWALCodec(serverVersion);
    }

-    private static boolean isCompatible(Long serverVersion) {
+    private static boolean isCompatible(Long serverVersion, StringBuilder buf) {


This might be out of scope of your JIRA, but it's a potentially small change. The method isCompatible looks pretty useless to me. Can't all the logic be moved down to MetaDataUtil.areClientAndServerCompatible?

ChinmaySKulkarni · 2020-02-11T19:50:38Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

        if (serverVersion == null) {
            return false;
        }
-        return MetaDataUtil.areClientAndServerCompatible(serverVersion);
+        return MetaDataUtil.areClientAndServerCompatible(serverVersion, buf);


I'm also not a big fan of passing the StringBuilder param everywhere. It might be just cleaner to have a small inner class to represent clientServerCompatibility and which contains perhaps the following:

client version

server version

an is compatible check based on the above 2 values

error message and/or code if there is an incompatibility

getters/setters of error message/code and versions

That also makes it obvious what the class does and what the buffer contains.
It shouldn't be too much work to add this small inner class. At least, we should rename "buf" to "errorString" or something.

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

ChinmaySKulkarni · 2020-02-27T22:06:44Z

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java

+
+                if (compatibility.getErrorCode() != 0) {
+                    if (compatibility.getErrorCode() == SQLExceptionCode.OUTDATED_JARS.getErrorCode()) {
+                        errorMessage.setLength(errorMessage.length()-1);


Do we need to setLength on the StringBuilder object? We can just directly call toString right?

The setLength(length-1) gets rid of a trailing semicolon if there are multiple outdated jars error messages strung together (for loop at line 1540)

If there aren't multiple messages, and the loop runs just once, does it print the correct thing still or leave out the last character?

@christinefeng ping on this one^

If it runs just once, there's still that trailing semicolon from line 1559, so the set length-1 gets rid of that semicolon
Not sure if I should add something similar to the incompatible_client_server_jar error message in case of multiple messages?

That's more for cases when we have SYSCAT on more than 1 region. Your change should be fine.

phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java

ChinmaySKulkarni

Small nit. Almost there @christinefeng !

phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java

ChinmaySKulkarni

+1, please squash all your commits into 1 and let's get a Hadoop QA run on your changes. Thanks @christinefeng !

…r with higher major version

stoty · 2023-08-01T14:56:11Z

Already merged.

christinefeng force-pushed the PHOENIX-5636 branch 2 times, most recently from f0d9879 to 0866ad8 Compare January 16, 2020 00:12

sguggilam reviewed Jan 31, 2020

View reviewed changes

christinefeng force-pushed the PHOENIX-5636 branch 4 times, most recently from e775f86 to ea39d9d Compare January 31, 2020 22:33

ChinmaySKulkarni reviewed Feb 11, 2020

View reviewed changes

christinefeng force-pushed the PHOENIX-5636 branch 4 times, most recently from e5c2ede to 6bd7d99 Compare February 19, 2020 22:24

ChinmaySKulkarni requested changes Feb 24, 2020

View reviewed changes

christinefeng force-pushed the PHOENIX-5636 branch 2 times, most recently from f7713fe to 5af0e6b Compare February 24, 2020 23:47

ChinmaySKulkarni requested changes Feb 27, 2020

View reviewed changes

ChinmaySKulkarni requested changes Feb 28, 2020

View reviewed changes

phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java Show resolved Hide resolved

ChinmaySKulkarni approved these changes Feb 28, 2020

View reviewed changes

christinefeng force-pushed the PHOENIX-5636 branch from 067bfb7 to 75af7e8 Compare February 28, 2020 00:36

PHOENIX-5636: Improve the error message when client connects to serve…

fa0c3a6

…r with higher major version

christinefeng force-pushed the PHOENIX-5636 branch from 75af7e8 to fa0c3a6 Compare February 28, 2020 00:40

stoty closed this Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-5636: Improve the error message when client connects to server with higher major version #676

PHOENIX-5636: Improve the error message when client connects to server with higher major version #676

christinefeng commented Jan 10, 2020

christinefeng commented Jan 10, 2020

sguggilam Jan 31, 2020

sguggilam Jan 31, 2020

ChinmaySKulkarni left a comment

ChinmaySKulkarni Feb 11, 2020

ChinmaySKulkarni Feb 11, 2020

ChinmaySKulkarni Feb 11, 2020

ChinmaySKulkarni Feb 11, 2020

ChinmaySKulkarni Feb 27, 2020

christinefeng Feb 27, 2020

ChinmaySKulkarni Feb 28, 2020

ChinmaySKulkarni Feb 28, 2020

christinefeng Feb 28, 2020

ChinmaySKulkarni Feb 28, 2020

ChinmaySKulkarni left a comment

ChinmaySKulkarni left a comment

stoty commented Aug 1, 2023

PHOENIX-5636: Improve the error message when client connects to server with higher major version #676

PHOENIX-5636: Improve the error message when client connects to server with higher major version #676

Conversation

christinefeng commented Jan 10, 2020

christinefeng commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni left a comment

Choose a reason for hiding this comment

ChinmaySKulkarni left a comment

Choose a reason for hiding this comment

stoty commented Aug 1, 2023