Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-5940 Pre-4.15 client cannot connect to 4.15+ server after SYS… - #950
PHOENIX-5940 Pre-4.15 client cannot connect to 4.15+ server after SYS…#950yanxinyi wants to merge 1 commit into
Conversation
virajjasani
left a comment
There was a problem hiding this comment.
One small request, looks good otherwise.
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Oct 30, 2020
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| ServerUtil.createIOException( | ||
| SchemaUtil.getPhysicalTableName(PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME_BYTES, | ||
| isTablesMappingEnabled).toString(), t)); | ||
| if (request.getClientVersion() >= MIN_SPLITTABLE_SYSTEM_CATALOG) { |
There was a problem hiding this comment.
I'm not so sure about this fix. If there is a legit problem loading SYSCAT region (can happen even if there is just 1 region), we won't be setting a syscat timestamp in this case. This in turn will allow weird cases like 4.14 client + 4.16 server + 4.13 metadata without throwing an UpgradeRequiredException.
How about we add an else block like this:
elseif (<syscatheaderrownotinregion>) {
// ignore
} else {
// this is a legit exception so set a controller exception with the same body as the if clause
}We can refactor the if and else since the if and last else have the same body
There was a problem hiding this comment.
@ChinmaySKulkarni
One question out of curiosity: when I checked CQSI invoking coprocessor hook for this call, version is set from client side:
GetVersionRequest.Builder builder =
GetVersionRequest.newBuilder();
builder.setClientVersion(
VersionUtil.encodeVersion(PHOENIX_MAJOR_VERSION,
PHOENIX_MINOR_VERSION, PHOENIX_PATCH_NUMBER));
instance.getVersion(controller, builder.build(), rpcCallback);
Hence, I thought it was always guaranteed to get clientVersion() from old client if old client is checking compatibility with new server.
However, looks like this check might not be reliable in all cases? In the above example, we might get 4.13 version by any chance (while checking for client 4.14 <-> server 4.16 compatibility?)
There was a problem hiding this comment.
That is the client jar version. The getVersion call does 3 things:
- Returns the phoenix server jar version + HBase version
- Embeds the namespace mapping prop in the response for 1.
- Returns the system catalog timestamp i.e. metadata version if syscat exists.
On the client-side after getting the response from the server, we check client-server compatibility based on these 3 things
Uh oh!
There was an error while loading. Please reload this page.
15a2dad to
65ba7fbComparestoty
commented
Oct 31, 2020
💔 -1 overall
This message was automatically generated. |
65ba7fb to
b6e7bc7CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@yanxinyi my assumption is there is no real diff in this class and is just a bunch of stuff copied from BackwardCompatibilityIT right?
There was a problem hiding this comment.
Yes, no logic changes. The only diff is adding extra param for zkQuorm or URL for the execute script.
Uh oh!
There was an error while loading. Please reload this page.
yanxinyi
commented
Nov 7, 2020
For additional testing on my local machine, I created more than 2k views, and the syscat split into 2 regions(confirmed by HBase UI page). Ran the select query from 4.14 and current 4.x clients without any issue. |
27a61dd to
97bc768Compare
ChinmaySKulkarni
left a comment
There was a problem hiding this comment.
+1. Please make sure the test failures aren't related to your changes and squash all commits before pushing. Thanks for fixing this @yanxinyi
stoty
commented
Nov 7, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 7, 2020
💔 -1 overall
This message was automatically generated. |
97bc768 to
5f6c961Compare
…TEM.CATALOG region has split
5f6c961 to
fb61748Comparestoty
commented
Nov 9, 2020
💔 -1 overall
This message was automatically generated. |
…TEM.CATALOG region has split