You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "XiaokaiWang (Code Review)" <ge...@cloudera.org> on 2019/10/05 17:01:46 UTC

[kudu-CR] KUDU-2587: support conditional update feature

XiaokaiWang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14377


Change subject: KUDU-2587: support conditional update feature
......................................................................

KUDU-2587: support conditional update feature

Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestConditionalUpdate.java
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/common/row_changelist.cc
M src/kudu/common/row_changelist.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet-decoder-eval-test.cc
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
42 files changed, 1,586 insertions(+), 66 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/14377/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
Gerrit-Change-Number: 14377
Gerrit-PatchSet: 1
Gerrit-Owner: XiaokaiWang <xi...@live.com>

[kudu-CR] KUDU-2587: support conditional update feature

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14377 )

Change subject: KUDU-2587: support conditional update feature
......................................................................


Patch Set 4:

From the looks of it, this is a pretty significant cross-cutting change. It's also got the potential to impact the data hot path, even for Kudu users who don't want to use the new feature.

I have a few requests:
- Split this up into several patches. The server-side impact is most important, and that's where we should focus the bulk of our review. You can move the  Java and C++ client changes to another patch (or multiple patches).
- Include a detailed commit message in each patch, but especially in the first patch. Explain any alternative implementations you considered, and why you chose to implement conditional updates in this way. When describing the implementation, try to refer to abstract Kudu concepts rather than to specific Kudu source files or classes.
- Show that the introduction of conditional updates does not regress performance for Kudu users that don't use them. You can use existing microbenchmarks in the code tree, and ideally one or more cluster performance benchmark (like YCSB).


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
Gerrit-Change-Number: 14377
Gerrit-PatchSet: 4
Gerrit-Owner: XiaokaiWang <xi...@live.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 07 Oct 2019 04:40:22 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2587: support conditional update feature

Posted by "XiaokaiWang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14377

to look at the new patch set (#4).

Change subject: KUDU-2587: support conditional update feature
......................................................................

KUDU-2587: support conditional update feature

Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestConditionalUpdate.java
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/common/row_changelist.cc
M src/kudu/common/row_changelist.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet-decoder-eval-test.cc
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tserver.proto
41 files changed, 1,603 insertions(+), 70 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/14377/4
-- 
To view, visit http://gerrit.cloudera.org:8080/14377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
Gerrit-Change-Number: 14377
Gerrit-PatchSet: 4
Gerrit-Owner: XiaokaiWang <xi...@live.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2587: support conditional update feature

Posted by "XiaokaiWang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14377

to look at the new patch set (#2).

Change subject: KUDU-2587: support conditional update feature
......................................................................

KUDU-2587: support conditional update feature

Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestConditionalUpdate.java
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/common/row_changelist.cc
M src/kudu/common/row_changelist.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet-decoder-eval-test.cc
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
42 files changed, 1,589 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/14377/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
Gerrit-Change-Number: 14377
Gerrit-PatchSet: 2
Gerrit-Owner: XiaokaiWang <xi...@live.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] KUDU-2587: support conditional update feature

Posted by "XiaokaiWang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/14377

to look at the new patch set (#3).

Change subject: KUDU-2587: support conditional update feature
......................................................................

KUDU-2587: support conditional update feature

Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
---
M java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestConditionalUpdate.java
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/common.proto
M src/kudu/common/row_changelist.cc
M src/kudu/common/row_changelist.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/wire_protocol.cc
M src/kudu/consensus/log-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/tablet-decoder-eval-test.cc
M src/kudu/tablet/tablet-test-util.h
M src/kudu/tablet/tablet.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/tserver.proto
41 files changed, 1,610 insertions(+), 70 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/14377/3
-- 
To view, visit http://gerrit.cloudera.org:8080/14377
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieebe6c7ece67e723c179a4c16444b59a63aa4612
Gerrit-Change-Number: 14377
Gerrit-PatchSet: 3
Gerrit-Owner: XiaokaiWang <xi...@live.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)