[kudu-CR] KUDU-3273: check null for cache entry

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17288 )

Change subject: KUDU-3273: check null for cache entry
......................................................................


Patch Set 5: Code-Review+1

(8 comments)

Thank you for the patch!  
I apologize for the delay in reviewing this patch.

Overall looks good, just a few nits.

http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG@7
PS5, Line 7: check null for cache entry
nit: how about

check result of TableLocationsCache.get() for null

?


http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG@9
PS5, Line 9: If get(key) is immediately
nit: how about

For TableLocationsCache, if get(key) is called immediately


http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG@11
PS5, Line 11: cacheTabletLocations(otherKey)
nit:

a call to cacheTabletLocations(otherKey)


http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG@11
PS5, Line 11: partition changes
nit:

partitions change


http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG@12
PS5, Line 12: cacheTabletLocations(key) and get(key)
nit:

calls to cacheTableLocations(key) and get(key)


http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG@12
PS5, Line 12: interleave
nit:

occurs


http://gerrit.cloudera.org:8080/#/c/17288/5//COMMIT_MSG@12
PS5, Line 12: it
nit:

the latter


http://gerrit.cloudera.org:8080/#/c/17288/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java:

http://gerrit.cloudera.org:8080/#/c/17288/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestTableLocationsCache.java@75
PS5, Line 75: public void testGet()
It would be great if you added a short description for this new test scenarios just to explain the essence of what it is designed to test.  It also makes sense to include a link to the upstream JIRA issue: you could find examples at https://github.com/apache/kudu/blob/28a3baab6149ee9317c71a4afc04ad4f3c8da2f9/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java#L202-L206 and https://github.com/apache/kudu/blob/28a3baab6149ee9317c71a4afc04ad4f3c8da2f9/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java#L253-L258



-- 
To view, visit http://gerrit.cloudera.org:8080/17288
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I13475701804ea4ae2bf8089a1e2b9143a12d2ab9
Gerrit-Change-Number: 17288
Gerrit-PatchSet: 5
Gerrit-Owner: Li Zhiming <li...@thinkingdata.cn>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 04 May 2021 04:48:25 +0000
Gerrit-HasComments: Yes