Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17376 )
Change subject: [util] Add special handling for empty strings in FastHash ...................................................................... Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/17376/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17376/4//COMMIT_MSG@17 PS4, Line 17: However if there is a mismatch : between Kudu client and Kudu server versions then : results are unexpected. This issue should be mentioned : in the Kudu release notes. I should have been more specific. This comment made me think more about the different client and server versions. Firstly we need to discuss cases where string is empty with seed=0(a) and otherwise(b) before this fix. (a) For seed=0, the FashHash for empty string evaluates to 0 and hence even if a string is present the client supplied BF will not have any bits set nor will the server side detect presence of the empty string resulting in the empty string being reported as not present. (b) For non-zero seed, empty string detection works correctly. Now lets consider different client and server versions after this fix with empty strings: (1) Newer server, older client, seed=0 Client doesn't set any bit in BF and newer server with magic value for empty string will not detect it returning absence of empty string. (2) Newer server, older client, non-zero seed The non-zero hashes won't match and the empty string will be reported as absent. (3) Newer client, older server, seed=0 Newer client would generate non-zero hash and set bits in BF but server generates 0 as hash value and incorrectly report absence of the empty string. (4) Newer client, older server, non-zero seed The non-zero hashes won't match and the empty string will be reported as not present. So overall, different client and server versions are problematic. > If needed we could add a feature flag to try and negotiate the compatible > behavior. I like this idea. Basically remove the existing RPC feature flag from client and servers (not from protobuf) and add a new flag in both client and servers. I'll update the diff with feature flag change and update the commit description. -- To view, visit http://gerrit.cloudera.org:8080/17376 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb2d401ab692decd5bc03b48a96d710546c0039e Gerrit-Change-Number: 17376 Gerrit-PatchSet: 4 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Mon, 10 May 2021 17:53:19 +0000 Gerrit-HasComments: Yes
