[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has posted comments on this change.
Change subject: KUDU-1811: C++ client: use larger batches when fetching scan
tokens
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7748/7/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:
Line 66: namespace {
> warning: do not use unnamed namespaces in header files [google-build-namesp
Done
--
To view, visit http://gerrit.cloudera.org:8080/7748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jun He
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7748 to look at the new patch set (#8). Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 31 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/8 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has posted comments on this change. Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. Patch Set 5: (1 comment) Thanks for the comments and PTAL. http://gerrit.cloudera.org:8080/#/c/7748/5/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: Line 390: void LookupTabletsByKeyOrNext(const KuduTable* table, > Yah I think it would be better to expose this as an additional parameter in Done -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7748 to look at the new patch set (#7). Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 34 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/7 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has posted comments on this change. Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. Patch Set 5: (1 comment) BTW, will this be included in 1.5 release? http://gerrit.cloudera.org:8080/#/c/7748/5/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: Line 390: void LookupTabletsByKeyOrNext(const KuduTable* table, > I don't quite follow - from looking at the C++ implementation, it's always Sorry for the confusion. What I meant was that kMaxReturnedTableLocations is internal and is not exposed to other external classes. It seems inconsistent to only expose kFetchTabletsPerRangeLookup as a public constant. Following the current the design of the Java implementation (e.g. getTabletsLocations), we can use a similar wrapper method (LookupTabletsByKeyOrNext) for token generator. Adding a new parameter to LookupTabletByKeyOrNext can allow us to set the max returned tablets per caller. But I still think it is better to make C++ and Java clients as consistent as possible. Let me know if we want to change Java implementation. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7748 to look at the new patch set (#6). Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 52 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/6 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has posted comments on this change.
Change subject: KUDU-1811: C++ client: use larger batches when fetching scan
tokens
..
Patch Set 5:
(4 comments)
I think some scale tests are needed to check the impact of the change (from 10
to 100). Additionally, it may be still valid to have different settings for
different sets of operations.
No matter which way we decide to go, I think we need to make Java and C++
client to share the similar logic and be consistent.
Please let me know your comment. Thanks.
http://gerrit.cloudera.org:8080/#/c/7748/5/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:
Line 76: /**
> style nit: we use // even for block comments
Done
Line 812: int requested_batch_size) {
> Might be good to name this 'max_returned_locations' to keep it consistent.
Done
Line 1048: string partition_key,
> nit: alignment
Done
http://gerrit.cloudera.org:8080/#/c/7748/5/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:
Line 390: void LookupTabletsByKeyOrNext(const KuduTable* table,
> Instead of creating a new method, it may be cleaner to add an optional argu
Thanks for the comments. I thought about it but went this way to make the C++
client implementation to be consistent with Java client.
Also, it followed the current design to hide the kMaxReturnedTableLocations
constant using wrapper methods (LookupTabletByKey and LookupTabletByKeyOrNext
in C++ and getTabletsLocations in Java) from token generator.
Please let me know your comment. Thanks.
--
To view, visit http://gerrit.cloudera.org:8080/7748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jun He
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has posted comments on this change. Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. Patch Set 5: Reran Jenkins build multiple times due to https://issues.apache.org/jira/browse/KUDU-1894. -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7748 to look at the new patch set (#5). Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 57 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/5 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7748 to look at the new patch set (#4). Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 57 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/4 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7748 to look at the new patch set (#3). Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 57 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/3 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has posted comments on this change. Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7748/2/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: Line 423:const int requested_batch_size); > warning: parameter 'requested_batch_size' is const-qualified in the functio Done -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7748 to look at the new patch set (#2). Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 57 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/2 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has posted comments on this change.
Change subject: KUDU-1811: C++ client: use larger batches when fetching scan
tokens
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7748/1/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:
Line 812: int requestedBatchSize) {
> warning: invalid case style for parameter 'requestedBatchSize' [readability
Done
http://gerrit.cloudera.org:8080/#/c/7748/1/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:
Line 421: Status ProcessLookupResponse(const LookupRpc& rpc,
> warning: function 'kudu::client::internal::MetaCache::ProcessLookupResponse
Done
Line 423:int requestedBatchSize);
> warning: invalid case style for parameter 'requestedBatchSize' [readability
Done
--
To view, visit http://gerrit.cloudera.org:8080/7748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He
Gerrit-Reviewer: Jun He
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1811: C++ client: use larger batches when fetching scan tokens
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/7748 Change subject: KUDU-1811: C++ client: use larger batches when fetching scan tokens .. KUDU-1811: C++ client: use larger batches when fetching scan tokens Improve C++ client scan token generation performance by fetching larger batches. Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e --- M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc 3 files changed, 57 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7748/1 -- To view, visit http://gerrit.cloudera.org:8080/7748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I79340dc9963944454770d82a2fbaba1b0c8a810e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-2089: Failed java tests can orphan test-tmp files
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/7719 Change subject: KUDU-2089: Failed java tests can orphan test-tmp files .. KUDU-2089: Failed java tests can orphan test-tmp files This changes the MiniKuduCluster code to add the paths to pathsToDelete before calling configureAndStartProcess. If configureAndStartProcess throws an exception, the test-tmp files will be still deleted during shutdown. Change-Id: I53c04987b6683dc959c319d384657c28b4671168 --- M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java 1 file changed, 6 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/7719/1 -- To view, visit http://gerrit.cloudera.org:8080/7719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I53c04987b6683dc959c319d384657c28b4671168 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. Patch Set 4: > (1 comment) > > I'm good with the change, just one javadoc comment to fix. Thanks! Thanks a lot for the comment! Fixed. -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Hello Jean-Daniel Cryans, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5723 to look at the new patch set (#4). Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. KUDU-1831. Java client does not check if the primary key columns are specified first This commit cleans up the java doc in Schema class, add some info to the java doc in AsyncKuduClient class, and add a test to cover the out of order primary key case. Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a --- M java/kudu-client/src/main/java/org/apache/kudu/Schema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java 3 files changed, 30 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5723/4 -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Hello Jean-Daniel Cryans, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5723 to look at the new patch set (#3). Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. KUDU-1831. Java client does not check if the primary key columns are specified first This commit cleans up the java doc in Schema class, add some info to the java doc in AsyncKuduClient class, and add a test to cover the out of order primary key case. Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a --- M java/kudu-client/src/main/java/org/apache/kudu/Schema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java 3 files changed, 32 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5723/3 -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/5723/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java: Line 238: @Test(timeout = 10) > No need to duplicate tests since KuduClient just wraps AsyncKuduClient. So Done -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. Patch Set 2: > (1 comment) > > Jun, what motivated you to make this change in the first place? It > seems that the master would just reply with the same error you're > adding in this patch, which is kind of duplicated code. Hi Jean-Daniel, thanks for the code review. I am thinking it is better to do the local client side validation before sending a request to the network. It might reduce some traffic to Kudu servers. As this is only needed in the createTable request, it is unlikely Kudu servers receive a huge amount of createTable requests. It makes sense to just let servers to handle it in this case. I will remove the check code in AsyncKuduClient, clean up Schema java doc, and remove the extra unit test. -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. Patch Set 2: Thanks for the discussion. Remove the new code from Schema class. Cleaned up the Javadoc. Added a check in createTable method to make sure the table schema is valid before sending rpc request. Added unit tests. -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Hello Jean-Daniel Cryans, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5723 to look at the new patch set (#2). Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. KUDU-1831. Java client does not check if the primary key columns are specified first This commit adds a check in createTable method at AsyncKuduClient to check if the primary key columns are specified first. If not, it throws an IllegalArgumentException. Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a --- M java/kudu-client/src/main/java/org/apache/kudu/Schema.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java 4 files changed, 60 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5723/2 -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. Patch Set 1: Based on https://github.com/apache/kudu/blob/master/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java#L295, the schema used by scanner seems to have no key at all. Am I right? If so, this change will fix the operation issues (e.g. insert using session.apply) without breaking the scanner. -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. Patch Set 1: > It seems like in the context of a projection, we don't want to > label any columns as keys, and thus this probably passes because we > don't propagate the "key" status from the table schema into the > projection schema. If this were causing a problem, wouldn't the > test added in the above-linked commit be failing now? If you think > the coverage is lacking, can you propose a new unit test which > fails with Jun's patch? > > Really the 'key(true)' vs 'key(false)' API is a bad one that we > should try to deprecate over time in favor of a 'setPrimaryKey(List)' > type API (we've moved towards this in the C++ client). This paves +1. I can add this in another change. > the way for actually allowing key columns not listed first in the > Schema object. Will we eventually remove the limit that key columns have to the first in both server (e.g. wire_protocol) and client? -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has posted comments on this change. Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. Patch Set 1: > I'm pretty sure we don't want this limitation, see > https://github.com/cloudera/kudu/commit/7d57b8b9204a0b170fc204cb7aaa98ad995cf635#diff-933a1ca025bc6af95c770d34958fe46a > > I wonder why the unit tests are still passing though. Thanks for the comments. It seems now java client relies on the server side (wire_protocol) to check if the schema is correct for operations, e.g. row insertion. For example, in TestScannerMultiTablet, if `val` column is added before `key2` at getSchema method, the test will fail during `setUpBeforeClass` when session apply a new row. The java client receives a NonRecoverableException saying "Got out-of-order key column ..." I think client side check is good as it can prevent the bad operation sent to the server. If schema is not the right place to add the check, how about doing the check within operation classes? -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1831. Java client does not check if the primary key columns are specified first
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5723 Change subject: KUDU-1831. Java client does not check if the primary key columns are specified first .. KUDU-1831. Java client does not check if the primary key columns are specified first This commit adds a check in Schema constructor to check if the primary key columns are specified first. If not, it throws an IllegalArgumentException. Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a --- M java/kudu-client/src/main/java/org/apache/kudu/Schema.java A java/kudu-client/src/test/java/org/apache/kudu/TestSchema.java 2 files changed, 52 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5723/1 -- To view, visit http://gerrit.cloudera.org:8080/5723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1ca2a572801c964331ed65c630db28436fcaf86a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-1771. Java client "connection refused" errors logged as "connection reset"
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5680 to look at the new patch set (#2). Change subject: KUDU-1771. Java client "connection refused" errors logged as "connection reset" .. KUDU-1771. Java client "connection refused" errors logged as "connection reset" This commit changes cleanup() to accepts a string input. It allows a caller to customize its error message based on the caller site context. Change-Id: If9eb40f11757ae76bc430b3f513b96592068d6e2 --- M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java 1 file changed, 10 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/5680/2 -- To view, visit http://gerrit.cloudera.org:8080/5680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9eb40f11757ae76bc430b3f513b96592068d6e2 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1771. Java client "connection refused" errors logged as "connection reset"
Jun He has posted comments on this change. Change subject: KUDU-1771. Java client "connection refused" errors logged as "connection reset" .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/5680/1/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java: PS1, Line 702: > Don't need this. Done PS1, Line 801: // > Can you re-align the comments? Done -- To view, visit http://gerrit.cloudera.org:8080/5680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If9eb40f11757ae76bc430b3f513b96592068d6e2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1771. Java client "connection refused" errors logged as "connection reset"
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5680 Change subject: KUDU-1771. Java client "connection refused" errors logged as "connection reset" .. KUDU-1771. Java client "connection refused" errors logged as "connection reset" This commit changes cleanup() to accepts a string input. It allows a caller to customize its error message based on the caller site context. Change-Id: If9eb40f11757ae76bc430b3f513b96592068d6e2 --- M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java 1 file changed, 8 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/5680/1 -- To view, visit http://gerrit.cloudera.org:8080/5680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If9eb40f11757ae76bc430b3f513b96592068d6e2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-1821. Noisy warning from catalog manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5637 to look at the new patch set (#3). Change subject: KUDU-1821. Noisy warning from catalog manager .. KUDU-1821. Noisy warning from catalog manager This commit changes the LOG if the catalog manager is trying to start. Add additional if statements for this case to show more details. Change-Id: I6a9b8577e4d857c63df6e66c10d6cf20270cd41e --- M src/kudu/master/catalog_manager.cc 1 file changed, 7 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/5637/3 -- To view, visit http://gerrit.cloudera.org:8080/5637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6a9b8577e4d857c63df6e66c10d6cf20270cd41e Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1821. Noisy warning from catalog manager
Jun He has posted comments on this change. Change subject: KUDU-1821. Noisy warning from catalog manager .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/5637/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS2, Line 392: l.catalog_status().posix_code() == CatalogManager::kStarting > Not following this. We don't set any posix code when we return ServiceUnava Yeah, you are right. I will change it to IsServiceUnavailable. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/5637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6a9b8577e4d857c63df6e66c10d6cf20270cd41e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1821. Noisy warning from catalog manager
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5637 to look at the new patch set (#2). Change subject: KUDU-1821. Noisy warning from catalog manager .. KUDU-1821. Noisy warning from catalog manager This commit changes the LOG if the catalog manager is trying to start. Add additional if statements for this case to show more details. Change-Id: I6a9b8577e4d857c63df6e66c10d6cf20270cd41e --- M src/kudu/master/catalog_manager.cc 1 file changed, 7 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/5637/2 -- To view, visit http://gerrit.cloudera.org:8080/5637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6a9b8577e4d857c63df6e66c10d6cf20270cd41e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1821. Noisy warning from catalog manager
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5637 Change subject: KUDU-1821. Noisy warning from catalog manager .. KUDU-1821. Noisy warning from catalog manager This commit changes the returned status to `Uninitialized` if the catalog manager is trying to start. Add additional if statements for this case to show more details. Change-Id: I6a9b8577e4d857c63df6e66c10d6cf20270cd41e --- M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/master/catalog_manager.cc 3 files changed, 15 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/5637/1 -- To view, visit http://gerrit.cloudera.org:8080/5637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6a9b8577e4d857c63df6e66c10d6cf20270cd41e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-1822. Update docs to build the documentation in ubuntu
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5608 Change subject: KUDU-1822. Update docs to build the documentation in ubuntu .. KUDU-1822. Update docs to build the documentation in ubuntu Include two packages, gem and ruby-dev, in the instruction to build documentations for Ubutnu. Change-Id: I474a0ffd61b4c50b0c0b5885a4e615679b4ae630 --- M docs/installation.adoc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/5608/1 -- To view, visit http://gerrit.cloudera.org:8080/5608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I474a0ffd61b4c50b0c0b5885a4e615679b4ae630 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-734. Add test coverage for encodings and strings
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5588 to look at the new patch set (#2). Change subject: KUDU-734. Add test coverage for encodings and strings .. KUDU-734. Add test coverage for encodings and strings Added a testAllPrimaryKeyTypes test to TestKeyEncoding class. It covers the case of setting/getting all of the available primary key types. Change-Id: Ia3fbeb0f889a4c454a48d11a6e42c7da0e58a329 --- M java/kudu-client/src/test/java/org/apache/kudu/client/TestKeyEncoding.java 1 file changed, 67 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/5588/2 -- To view, visit http://gerrit.cloudera.org:8080/5588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia3fbeb0f889a4c454a48d11a6e42c7da0e58a329 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-734. Add test coverage for encodings and strings
Jun He has posted comments on this change.
Change subject: KUDU-734. Add test coverage for encodings and strings
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5588/1//COMMIT_MSG
Commit Message:
Line 9: added a TestEncodings class to check various encodings for all column
types
> I'm not sure this belongs in the Java client tests. We already have similar
Thanks for the explanation.
I found that `TestRowResult` has covered setting/getting all of the available
column types.
This test improved the coverage by setting/getting all of the available primary
key types, which is not covered by TestKeyEncoding.
I think I can merge it into TestKeyEncoding class.
I will push the new change.
http://gerrit.cloudera.org:8080/#/c/5588/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestStrings.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestStrings.java:
Line 51: public void testMultiStringColumnsInsert() throws Exception {
> I think this is already covered by TestKuduClient#testStrings() (the right-
Done
Line 90: public void testUTF8StringEncoding() throws Exception {
> I think this is already covered by TestKuduClient#testUTF8 added in df02bb4
Not a problem. I will remove it.
--
To view, visit http://gerrit.cloudera.org:8080/5588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia3fbeb0f889a4c454a48d11a6e42c7da0e58a329
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He
Gerrit-Reviewer: Jun He
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-90. Add a header checksum to our RPC protocol
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5578 to look at the new patch set (#4). Change subject: KUDU-90. Add a header checksum to our RPC protocol .. KUDU-90. Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. The new message framing will be total_size: (32-bit big-endian integer) the size of the rest of the message, not including this 4-byte header header_checksum: (32-bit big-endian integer) CRC-32C the checksum of header (excluding varint-prefixed header size field) - CRC-32C computation in C++ uses `kudu/util/crc.h`. - CRC-32C computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32C` header: varint-prefixed header protobuf - client->server messages use the RequestHeader protobuf - server->client messages use the ResponseHeader protobuf ... This is a non-backward compatible change. Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 --- M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32C.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java M src/kudu/rpc/constants.h M src/kudu/rpc/serialization.cc 6 files changed, 763 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/4 -- To view, visit http://gerrit.cloudera.org:8080/5578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total size.
Jun He has posted comments on this change.
Change subject: [KUDU-90] Add a header checksum to our RPC protocol Added
4-byte checksum into the RPC protocol. Checksum is inserted immediately after
total_size.
..
Patch Set 3:
(5 comments)
As the cost of computing CRC-32C is quite low, we may still consider to have it
if TLS is only optional.
http://gerrit.cloudera.org:8080/#/c/5578/3//COMMIT_MSG
Commit Message:
Line 7: [KUDU-90] Add a header checksum to our RPC protocol
> nit: follow the format used by other commit messages (KUDU-90. Add a header
Done
Line 8: Added 4-byte checksum into the RPC protocol.
> nit: please add a blank line before this one
Done
Line 25: This is a non-backward compatible change.
> I think it's too late to make a non-backward-compatible change. We should f
For the checksum, I think it makes sense to have the checksum for the whole
message.
For the compatibility issue, a straightforward way is to add an optional field
in pb header schema to store CRC. But this will complicate the checksum
computation and also will add 4-byte data in header object.
Feature flags sounds like another good way to solve the compatibility issue.
If TLS will be always enabled, we don't need additional CRC. But if it is
optional, I think CRC is still valuable if we only care network error instead
of integrity.
So if TLS is enabled, we can skip CRC, otherwise, we add CRC for the new
protocol. I think enabling TLS will face the similar compatibility problem.
Please let me know your comments. Thanks.
http://gerrit.cloudera.org:8080/#/c/5578/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java:
Line 74: public static final int INT_SIZE_IN_BYTES = 4;
> Integer.SIZE / 8
Done
http://gerrit.cloudera.org:8080/#/c/5578/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java:
Line 26: public class TestRpcChecksum {
> this is nice as a unit test but would be good to actually inject invalid ch
This is a great idea. Will add it once we decide how to proceed.
--
To view, visit http://gerrit.cloudera.org:8080/5578
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jun He
Gerrit-Reviewer: Jun He
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1655. Update docs for ASF maven repository coordinates
Jun He has posted comments on this change. Change subject: KUDU-1655. Update docs for ASF maven repository coordinates .. Patch Set 2: (3 comments) Addressed the review comments. http://gerrit.cloudera.org:8080/#/c/5581/1//COMMIT_MSG Commit Message: Line 7: KUDU-1655. Update docs for ASF maven repository coordinates > nit: please add a blank line between the first (summary) line and the detai Done http://gerrit.cloudera.org:8080/#/c/5581/1/docs/developing.adoc File docs/developing.adoc: Line 82: are also now available via the link:http://repository.apache.org[ASF Maven repository] and > It looks like when we publish to the ASF maven it actually propagates to Ma Done Line 95: Use the kudu-spark_2.10 artifact if using Spark with Scala 2.10 > should we note something like "If you are using Spark 2 with Scala 2.11, us Done -- To view, visit http://gerrit.cloudera.org:8080/5581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ddafd84dbf2a977e8b7faea78d3b38968c4e19e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1655. Update docs for ASF maven repository coordinates
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5581 to look at the new patch set (#2). Change subject: KUDU-1655. Update docs for ASF maven repository coordinates .. KUDU-1655. Update docs for ASF maven repository coordinates - Update Maven Artifacts - Update Kudu Integration with Spark using --packages Change-Id: I4ddafd84dbf2a977e8b7faea78d3b38968c4e19e --- M docs/developing.adoc 1 file changed, 18 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/5581/2 -- To view, visit http://gerrit.cloudera.org:8080/5581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4ddafd84dbf2a977e8b7faea78d3b38968c4e19e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-734. Add test coverage for encodings and strings
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5588 Change subject: KUDU-734. Add test coverage for encodings and strings .. KUDU-734. Add test coverage for encodings and strings added a TestEncodings class to check various encodings for all column types * Encoding Types * |=== * | Column Type | Encoding * | int8, int16, int32 | plain, bitshuffle, run length * | int64, unixtime_micros | plain, bitshuffle * | float, double | plain, bitshuffle * | bool| plain, run length * | string, binary | plain, prefix, dictionary * |=== added another TestStrings class to cover 1. test insert string columns in a few different orders other than left-to-right 2. test proper UTF8 strings outside of ASCII (using chinese characters) Change-Id: Ia3fbeb0f889a4c454a48d11a6e42c7da0e58a329 --- A java/kudu-client/src/test/java/org/apache/kudu/client/TestEncodings.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestStrings.java 2 files changed, 319 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/5588/1 -- To view, visit http://gerrit.cloudera.org:8080/5588 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia3fbeb0f889a4c454a48d11a6e42c7da0e58a329 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-1817. Fix kudu-spark2 artifactId in pom.xml
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5586 Change subject: KUDU-1817. Fix kudu-spark2 artifactId in pom.xml .. KUDU-1817. Fix kudu-spark2 artifactId in pom.xml kudu-spark2 pom.xml is malformed due to a maven shade plugin issue: dependency-reduced-pom should use shadedArtifactId (https://issues.apache.org/jira/browse/MSHADE-155) I upgraded maven-shade-plugin to version 2.4 and fixed the pom.xml. Additionally, I moved the default properties to a default profile. This makes the property settings consistent for both versions. Change-Id: I0dc9ac261a82e13d189bc1be04faea5a9812d518 --- M java/kudu-spark/pom.xml 1 file changed, 15 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/5586/1 -- To view, visit http://gerrit.cloudera.org:8080/5586 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I0dc9ac261a82e13d189bc1be04faea5a9812d518 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] KUDU-1655. Update docs for ASF maven repository coordinates - Update Maven Artifacts - Update Kudu Integration with Spark using --packages
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5581 Change subject: KUDU-1655. Update docs for ASF maven repository coordinates - Update Maven Artifacts - Update Kudu Integration with Spark using --packages .. KUDU-1655. Update docs for ASF maven repository coordinates - Update Maven Artifacts - Update Kudu Integration with Spark using --packages Change-Id: I4ddafd84dbf2a977e8b7faea78d3b38968c4e19e --- M docs/developing.adoc 1 file changed, 8 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/5581/1 -- To view, visit http://gerrit.cloudera.org:8080/5581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I4ddafd84dbf2a977e8b7faea78d3b38968c4e19e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total size.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5578 to look at the new patch set (#3). Change subject: [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. .. [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. The new message framing will be total_size: (32-bit big-endian integer) the size of the rest of the message, not including this 4-byte header header_checksum: (32-bit big-endian integer) CRC-32C the checksum of header (excluding varint-prefixed header size field) - CRC-32C computation in C++ uses `kudu/util/crc.h`. - CRC-32C computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32C` header: varint-prefixed header protobuf - client->server messages use the RequestHeader protobuf - server->client messages use the ResponseHeader protobuf ... This is a non-backward compatible change. Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 --- M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32C.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java M src/kudu/rpc/constants.h M src/kudu/rpc/serialization.cc 6 files changed, 763 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/3 -- To view, visit http://gerrit.cloudera.org:8080/5578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total size.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5578 to look at the new patch set (#2). Change subject: [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. .. [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. The new message framing will be total_size: (32-bit big-endian integer) the size of the rest of the message, not including this 4-byte header and also not including 4-byte checksum header_checksum: (32-bit big-endian integer) CRC-32 the checksum of header (excluding varint-prefixed header size field) - CRC-32 computation in C++ uses `boost/crc.hpp`. - CRC-32 computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32` header: varint-prefixed header protobuf - client->server messages use the RequestHeader protobuf - server->client messages use the ResponseHeader protobuf ... This is a non-backward compatible change. Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 --- M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java M src/kudu/rpc/constants.h M src/kudu/rpc/serialization.cc 6 files changed, 758 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/2 -- To view, visit http://gerrit.cloudera.org:8080/5578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] fix lint error
Jun He has abandoned this change. Change subject: fix lint error .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/5579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I2d10f7b5aa370cd4affbd7cf0055139988374341 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] fix lint error
Jun He has restored this change. Change subject: fix lint error .. Restored -- To view, visit http://gerrit.cloudera.org:8080/5579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: restore Gerrit-Change-Id: I2d10f7b5aa370cd4affbd7cf0055139988374341 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] fix lint error
Jun He has abandoned this change. Change subject: fix lint error .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/5579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I2d10f7b5aa370cd4affbd7cf0055139988374341 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] fix lint error
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5579 Change subject: fix lint error .. fix lint error Change-Id: I2d10f7b5aa370cd4affbd7cf0055139988374341 --- M src/kudu/rpc/serialization.cc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/5579/1 -- To view, visit http://gerrit.cloudera.org:8080/5579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2d10f7b5aa370cd4affbd7cf0055139988374341 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
[kudu-CR] [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total size.
Jun He has uploaded a new change for review. http://gerrit.cloudera.org:8080/5578 Change subject: [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. .. [KUDU-90] Add a header checksum to our RPC protocol Added 4-byte checksum into the RPC protocol. Checksum is inserted immediately after total_size. The new message framing will be total_size: (32-bit big-endian integer) the size of the rest of the message, not including this 4-byte header and also not including 4-byte checksum header_checksum: (32-bit big-endian integer) CRC-32 the checksum of header (excluding varint-prefixed header size field) - CRC-32 computation in C++ uses `boost/crc.hpp`. - CRC-32 computation in Java is based on hadoop-common class, `org.apache.hadoop.util.PureJavaCrc32` header: varint-prefixed header protobuf - client->server messages use the RequestHeader protobuf - server->client messages use the ResponseHeader protobuf ... This is a non-backward compatible change. Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 --- M java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java A java/kudu-client/src/main/java/org/apache/kudu/util/Crc32.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcChecksum.java M src/kudu/rpc/constants.h M src/kudu/rpc/serialization.cc 6 files changed, 758 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/5578/1 -- To view, visit http://gerrit.cloudera.org:8080/5578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Icf71ed4d4ac924445606b0d6fcf477d025243c74 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jun He
