[kudu-CR] Add time/watermark based garbage collection to ResultTracker
Todd Lipcon has posted comments on this change. Change subject: Add time/watermark based garbage collection to ResultTracker .. Patch Set 13: looking over this stuff tonight, just a note: seems like this is lacking actually _triggering_ the GC. I guess we either need some new thread or the ability to do it inline based on memtracker consumption -- To view, visit http://gerrit.cloudera.org:8080/3628 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Alexey Serbin has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3868/5/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
Line 41: Status (KuduPartialRow::* pmfSetter)(int, const Slice&),
> Would you mind using std::function for this? Easier to read. Also, minor st
Done
Line 41: Status (KuduPartialRow::* pmfSetter)(int, const Slice&),
> could this be a std::function?
It could be, but there will be more typedefs which looks exactly this this.
Will do.
Line 43: int column_idx, bool should_copy) {
> nit: please use an enum instead of a bool for should_copy, like enum { COPY
Done
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1533 Spark Kudu Rdd/Dataframe upsert
Will Berkeley has posted comments on this change.
Change subject: KUDU-1533 Spark Kudu Rdd/Dataframe upsert
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3871/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
File
java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala:
Line 61: new KuduRelation(tableName, kuduMaster)(sqlContext)
> Should the upsert flag get passed here as well?
N/A. See below.
Line 78: override def createRelation(sqlContext: SQLContext, mode: SaveMode,
> How would you feel about removing the method and not implementing Creatable
You're right we shouldn't implement CreatableRelationProvider if we can't do it
right. Alternatively, couldn't createRelation take a dataframe, construct a
Kudu schema from its schema, and then allow it to be persisted to Kudu? Working
this way, we could support everything except Overwrite right now...but it's
going to be really awkward to specify the key and the partition schema. Awkward
enough I think we should remove creatableRelationProvider until we can
implement it correctly as it is now (based on an existing table).
All the operations will still be available on a dataframe via KuduContext.
Line 94: case Overwrite => kuduRelation.insert(data, overwrite = true)
> Throw Unsupported here as well for the reasons below.
N/A. See above.
Line 218: context.writeRows(data, tableName, overwrite, upsert)
> Could you actually change this to throw a NotSupported exception when overw
Done
http://gerrit.cloudera.org:8080/#/c/3871/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
File
java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala:
Line 132: def writeRows(data: DataFrame, tableName: String, overwrite:
Boolean, upsert: Boolean) {
> Instead of writeRows taking two booleans, could you split this into insertR
Done
Line 151: def writeRows(rows: Iterator[Row],
> Same here. There shouldn't be too much code duplication if you pull out th
Done
--
To view, visit http://gerrit.cloudera.org:8080/3871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Chris George
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1533 Spark Kudu Rdd/Dataframe upsert
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1533 Spark Kudu Rdd/Dataframe upsert .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2785/ -- To view, visit http://gerrit.cloudera.org:8080/3871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Chris George Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1533 Spark Kudu Rdd/Dataframe upsert
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3871
to look at the new patch set (#2).
Change subject: KUDU-1533 Spark Kudu Rdd/Dataframe upsert
..
KUDU-1533 Spark Kudu Rdd/Dataframe upsert
This patch improves the Kudu SparkSQL integration in two ways:
1) Removed trait creatableRelationProvider from DefaultSource. This
is an improvement because this trait cannot be correctly implemented
for Kudu without support for table truncation.
2) Added {insert, update, upsert, delete}Rows methods to KuduContext.
This replaces and expands on the removed functionality of
DefaultSource.
The change is backwards-incompatible. Syntax like
df.write.options(newOptions).mode("append").kudu
no longer works, and now needs to be written like
kuduContext.insertRows(df, tableName)
Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala
M
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
4 files changed, 100 insertions(+), 93 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/3871/2
--
To view, visit http://gerrit.cloudera.org:8080/3871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Chris George
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add table id to AlterTableResponsePB
Dan Burkert has submitted this change and it was merged. Change subject: Add table id to AlterTableResponsePB .. Add table id to AlterTableResponsePB This also changes the Java client to take advantage of the ID to selectively clear the table locations cache on range partitioning alteration. Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Reviewed-on: http://gerrit.cloudera.org:8080/3859 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 5 files changed, 44 insertions(+), 9 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Fix bug in partition key debug printing
Dan Burkert has submitted this change and it was merged. Change subject: Fix bug in partition key debug printing .. Fix bug in partition key debug printing This fixes a bug in compound-column range partition key debug printing. Also adds a test for range partition encoding with compound columns. Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Reviewed-on: http://gerrit.cloudera.org:8080/3879 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc M src/kudu/common/partition.h 3 files changed, 110 insertions(+), 58 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Memory tracking for result tracker
Todd Lipcon has posted comments on this change. Change subject: Memory tracking for result tracker .. Patch Set 19: (1 comment) http://gerrit.cloudera.org:8080/#/c/3627/19/src/kudu/rpc/result_tracker.h File src/kudu/rpc/result_tracker.h: Line 231: return client_state_map_bytes_; I find this somewhat confusing vs just using a MemTrackerAllocator on the client_state_map_. Adar, since you looked at this earlier, do you understand this part of the patch? Why not just attach the allocators to the MemTracker? -- To view, visit http://gerrit.cloudera.org:8080/3627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625 Gerrit-PatchSet: 19 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Fix bug in partition key debug printing
Kudu Jenkins has posted comments on this change. Change subject: Fix bug in partition key debug printing .. Patch Set 3: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2784/ -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] WIP: fix issue where we write to wrong tablet
Todd Lipcon has abandoned this change. Change subject: WIP: fix issue where we write to wrong tablet .. Abandoned I don't think this is relevant now given various restructuring that has happened in both the client and the master -- To view, visit http://gerrit.cloudera.org:8080/939 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: Iccd10b2f5859c669ece2f3284c3630421a194efe Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon
[kudu-CR] KUDU-526: use on-disk cmeta when loading existing master state
Kudu Jenkins has posted comments on this change. Change subject: KUDU-526: use on-disk cmeta when loading existing master state .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2782/ -- To view, visit http://gerrit.cloudera.org:8080/3786 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b4c6d8b6adf696973445a6f9d1314ba9de27e70 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Hello Mike Percy, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/3880 to review the following change. Change subject: KUDU-1474: single to multi-master deployment migration .. KUDU-1474: single to multi-master deployment migration This patch introduces the machinery needed to migrate from a single-node master deployment to multi-master: 1. Inclusion of the tablet copy service in the master. This was easy; the master module already depends on 'tserver', so registering the service is just a few lines of code. 2. The new "kudu" command line tool, whose operations are used in migration. It'll also serve as the basis for KUDU-619. 3. An end-to-end integration test to showcase the migration. A couple notes about the new tool. I began with a gflags-based approach, but found it unwieldy after a while, because it's tough to provide a good help experience, and the lack of positional argument support makes for long command lines. I briefly looked at boost but didn't want to reintroduce a library dependency, so I ended up writing my own (simple) parser. It maps command line arguments to "actions" and allows for arbitrary levels of nesting, so you can do things like "kudu tablet copy blah" as well as "kudu fs dump". At each level, if insufficient arguments are provided, an informative help message is printed though there's room for improvement here. Change-Id: I89c741381ced3731736228cd07fe85106ae72541 --- M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/external_mini_cluster.h A src/kudu/integration-tests/master_migration-itest.cc M src/kudu/master/master.cc M src/kudu/master/sys_catalog.cc M src/kudu/master/sys_catalog.h M src/kudu/tools/CMakeLists.txt A src/kudu/tools/tool_actions.cc A src/kudu/tools/tool_actions.h A src/kudu/tools/tool_actions_fs.cc A src/kudu/tools/tool_actions_fs.h A src/kudu/tools/tool_actions_tablet.cc A src/kudu/tools/tool_actions_tablet.h A src/kudu/tools/tool_main.cc 14 files changed, 944 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3880/1 -- To view, visit http://gerrit.cloudera.org:8080/3880 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1474: single to multi-master deployment migration
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1474: single to multi-master deployment migration .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2783/ -- To view, visit http://gerrit.cloudera.org:8080/3880 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Fix bug in partition key debug printing
Adar Dembo has posted comments on this change. Change subject: Fix bug in partition key debug printing .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Adar Dembo has posted comments on this change. Change subject: Add table id to AlterTableResponsePB .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Kudu Jenkins has posted comments on this change. Change subject: Add table id to AlterTableResponsePB .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2781/ -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Dan Burkert has posted comments on this change.
Change subject: Add table id to AlterTableResponsePB
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3859/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 375: }).addErrback(new Callback() {
> Aren't these of type Void? Or Exception?
Done
--
To view, visit http://gerrit.cloudera.org:8080/3859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Add table id to AlterTableResponsePB
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3859 to look at the new patch set (#4). Change subject: Add table id to AlterTableResponsePB .. Add table id to AlterTableResponsePB This also changes the Java client to take advantage of the ID to selectively clear the table locations cache on range partitioning alteration. Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 5 files changed, 44 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/3859/4 -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Fix bug in partition key debug printing
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3879 to look at the new patch set (#3). Change subject: Fix bug in partition key debug printing .. Fix bug in partition key debug printing This fixes a bug in compound-column range partition key debug printing. Also adds a test for range partition encoding with compound columns. Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc M src/kudu/common/partition.h 3 files changed, 110 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/3879/3 -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Fix bug in partition key debug printing
Kudu Jenkins has posted comments on this change. Change subject: Fix bug in partition key debug printing .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2780/ -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Adar Dembo has posted comments on this change.
Change subject: Add table id to AlterTableResponsePB
..
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3859/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 375: }).addErrback(new Callback() {
Aren't these of type Void? Or Exception?
--
To view, visit http://gerrit.cloudera.org:8080/3859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Fix bug in partition key debug printing
Adar Dembo has posted comments on this change. Change subject: Fix bug in partition key debug printing .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Fix bug in partition key debug printing
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3879 to look at the new patch set (#2). Change subject: Fix bug in partition key debug printing .. Fix bug in partition key debug printing This fixes a bug in compound-column range partition key debug printing. Also adds a test for range partition encoding with compound columns. Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc M src/kudu/common/partition.h 3 files changed, 103 insertions(+), 45 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/3879/2 -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Fix bug in partition key debug printing
Kudu Jenkins has posted comments on this change. Change subject: Fix bug in partition key debug printing .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2779/ -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Fix bug in partition key debug printing
Adar Dembo has posted comments on this change. Change subject: Fix bug in partition key debug printing .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3868
to look at the new patch set (#6).
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
[KuduPartialRow::Set{Binary,String}()] copy input data
KuduPartialRow::SetBinary()/SetString() behavior was optimized
to omit copying of the passed data. However, a user of the API
might assume these methods are safe to use along with other setters
as SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, the behavior of these methods has been changed
to immediately copy the input data.
This is a safe modification, but it is not backward-compatible
in semver notation since it changes the semantics
of already existing methods. However, we don't bump API version
since Kudu is not 1.0 yet. As for the ABI, it remains compatible
with the prior versions: the existing client C++ code
will still compile and link.
This approach may add some performance regression issues for
already existing clients, but hopefully it is negligible for most
of the C++ clients around. As for the Impala-Kudu integration,
it seems current Impala code uses SetString() only in one place
at be/src/sec/kudu-table-sink.cc, and that can be addressed
separately.
An alternative approach might be to deprecate
KuduPartialRow::Set{Binary,String}() in favor of the newly introduced
KuduPartialRow::Set{Binary,String}NoCopy() methods. That approach
would spare us from performance regression worries and compatibility
issues. However, after some discussion, introducing the copying
behavior of KuduPartialRow::SetBinary()/SetString() methods
appeared to be more beneficial in the long run
from the usability perspective.
Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 211 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/3868/6
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Kudu Jenkins has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 6:
Build Started http://104.196.14.100/job/kudu-gerrit/2778/
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: No
[kudu-CR] [java client] Support add/remove partition
Dan Burkert has submitted this change and it was merged. Change subject: [java client] Support add/remove partition .. [java client] Support add/remove partition This also sneaks a fix into catalog manager to change the status type when rejecting invalid alter table partitioning operations. Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Reviewed-on: http://gerrit.cloudera.org:8080/3854 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java D java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeCache.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java A java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.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/TestAsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/master/catalog_manager.cc 13 files changed, 879 insertions(+), 294 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add table id to AlterTableResponsePB
Dan Burkert has posted comments on this change.
Change subject: Add table id to AlterTableResponsePB
..
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3859/2/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java:
Line 26: String tableId;
> private?
Done
Line 37:* @return the ID of the altered table, or null if the master
version is too old (< 0.10)
> I think we should avoid embedding version numbers directly in the code like
Done
http://gerrit.cloudera.org:8080/#/c/3859/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 361: // version to return the table ID (>= 0.10), we can
selectively clear
> Nit: version string here too.
Done
Line 365: if (resp instanceof AlterTableResponse) {
> The type erasure here is annoying. The only way to avoid it would be to dup
Done
--
To view, visit http://gerrit.cloudera.org:8080/3859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Fix bug in partition key debug printing
Hello Adar Dembo, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/3879 to review the following change. Change subject: Fix bug in partition key debug printing .. Fix bug in partition key debug printing This fixes a bug in compound-column range partition key debug printing. Also adds a test for range partition encoding with compound columns. Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc M src/kudu/common/partition.h 3 files changed, 96 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/3879/1 -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add table id to AlterTableResponsePB
Kudu Jenkins has posted comments on this change. Change subject: Add table id to AlterTableResponsePB .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2776/ -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Fix bug in partition key debug printing
Kudu Jenkins has posted comments on this change. Change subject: Fix bug in partition key debug printing .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2777/ -- To view, visit http://gerrit.cloudera.org:8080/3879 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idac701428a9af03605cd1a156926bdbd1a5f5fac Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3859 to look at the new patch set (#3). Change subject: Add table id to AlterTableResponsePB .. Add table id to AlterTableResponsePB This also changes the Java client to take advantage of the ID to selectively clear the table locations cache on range partitioning alteration. Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 5 files changed, 43 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/3859/3 -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [java client] Support add/remove partition
Adar Dembo has posted comments on this change. Change subject: [java client] Support add/remove partition .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Adar Dembo has posted comments on this change.
Change subject: Add table id to AlterTableResponsePB
..
Patch Set 2:
(4 comments)
Will you be making a similar change to the C++ client?
http://gerrit.cloudera.org:8080/#/c/3859/2/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java:
Line 26: String tableId;
private?
Line 37:* @return the ID of the altered table, or null if the master
version is too old (< 0.10)
I think we should avoid embedding version numbers directly in the code like
this. "Returns the table ID or null if the master was too old" is sufficient.
It's kind of a capabilities vs. versions thing; if it's desirable for users to
know whether a piece of functionality exists, we should expose it via a
capability or feature flag.
http://gerrit.cloudera.org:8080/#/c/3859/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 361: // version to return the table ID (>= 0.10), we can
selectively clear
Nit: version string here too.
Line 365: if (resp instanceof AlterTableResponse) {
The type erasure here is annoying. The only way to avoid it would be to
duplicate the callback for addErrback(), right? Maybe we can do that without
duplicating all of the code that the two invoke?
--
To view, visit http://gerrit.cloudera.org:8080/3859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Mike Percy has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3868/5/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
Line 41: Status (KuduPartialRow::* pmfSetter)(int, const Slice&),
Would you mind using std::function for this? Easier to read. Also, minor style
nit: don't use camelCaps for variable names.
Line 43: int column_idx, bool should_copy) {
nit: please use an enum instead of a bool for should_copy, like enum { COPY,
NO_COPY } or { kShouldCopy, kShouldNotCopy }, something like that. This makes
it obvious what the parameter meaning is at the call site.
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata
Dan Burkert has posted comments on this change.
Change subject: KUDU-1500: Data race in
RaftConsensusITest.TestCorruptReplicaMetadata
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3823/6/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:
Line 109: const Partition partition_locked() const {
Is this the right return type? Seems weird to return const by value.
Line 142: const PartitionSchema partition_schema_locked() const {
same here.
--
To view, visit http://gerrit.cloudera.org:8080/3823
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Dan Burkert has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3868/5/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
Line 41: Status (KuduPartialRow::* pmfSetter)(int, const Slice&),
could this be a std::function? I
find this syntax pretty difficult.
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up
Kudu Jenkins has posted comments on this change. Change subject: KUDU-763 consensus queue metrics on followers are messed up .. Patch Set 8: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2775/ -- To view, visit http://gerrit.cloudera.org:8080/3501 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Kudu Jenkins has posted comments on this change. Change subject: Add table id to AlterTableResponsePB .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2774/ -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java client] Support add/remove partition
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3854 to look at the new patch set (#3). Change subject: [java client] Support add/remove partition .. [java client] Support add/remove partition This also sneaks a fix into catalog manager to change the status type when rejecting invalid alter table partitioning operations. Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java D java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeCache.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java A java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.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/TestAsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/master/catalog_manager.cc 13 files changed, 879 insertions(+), 294 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/3854/3 -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [java client] Support add/remove partition
Kudu Jenkins has posted comments on this change. Change subject: [java client] Support add/remove partition .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2773/ -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add table id to AlterTableResponsePB
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3859 to look at the new patch set (#2). Change subject: Add table id to AlterTableResponsePB .. Add table id to AlterTableResponsePB This also changes the Java client to take advantage of the ID to selectively clear the table locations cache on range partitioning alteration. Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto 5 files changed, 37 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/3859/2 -- To view, visit http://gerrit.cloudera.org:8080/3859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I63402581fbdaef2abdf37910c1dfaedbdcba76e2 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [java client] Support add/remove partition
Dan Burkert has posted comments on this change.
Change subject: [java client] Support add/remove partition
..
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java:
Line 118:* Multiple range partitions may be added as part of a single alter
table transaction by calling
> OK, fair. Truth be told, the wait() functionality that's built into the Kud
It's a separate call for the Java client, but yes.
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
PS1, Line 1380: // This is making this tablet available
: // Even if two clients were racing in this method they
are putting the same RemoteTablet
: // with the same start key in the CSLM in the end
> I think you missed this.
Done
http://gerrit.cloudera.org:8080/#/c/3854/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 349: public Deferred alterTable(String name,
AlterTableOptions ato) {
> Why drop the generic type?
Done
Line 356: // Clear the caches so the new partition is immediately visible.
> Nit: update this comment.
Done
Line 1350: if (existingLocationsCache != null) {
> Nit: terminate with period.
Done
Line 1361:
> Nit: "Build the list..."
Done
Line 1362: // If we already know about this one, just refresh the
locations
> Nit: "its"
Done
Line 1365: currentTablet.refreshTabletClients(tabletPb);
> Nit: terminate with period.
Done
Line 1370: // Putting it here first doesn't make it visible because
tabletsCache is always looked up
> As we discussed, it looks like there's an opportunity to get rid of tablet2
This ended up being sufficiently tricky that I'm punting as part of this
change. The issue is that by moving it, we have to build of the remote tablets
under the tablet locations cache lock, which involves DNS resolution.
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java:
Line 282: String tableName = name.getMethodName();
> Thanks. Could you fix it for the other tests in this file too?
Done
--
To view, visit http://gerrit.cloudera.org:8080/3854
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR](gh-pages) www: Add Kudu talks to Community page
Mike Percy has submitted this change and it was merged. Change subject: www: Add Kudu talks to Community page .. www: Add Kudu talks to Community page Change-Id: I26fde3dabd5f08ac656f286118704bec82f27877 Reviewed-on: http://gerrit.cloudera.org:8080/3877 Reviewed-by: Mike Percy Tested-by: Mike Percy --- M community.md 1 file changed, 9 insertions(+), 3 deletions(-) Approvals: Mike Percy: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/3877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I26fde3dabd5f08ac656f286118704bec82f27877 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: Mike Percy Gerrit-Reviewer: Mike Percy
[kudu-CR](gh-pages) www: Add Kudu talks to Community page
Mike Percy has posted comments on this change. Change subject: www: Add Kudu talks to Community page .. Patch Set 1: Code-Review+2 Verified+1 Verified this looks good, pushing -- To view, visit http://gerrit.cloudera.org:8080/3877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I26fde3dabd5f08ac656f286118704bec82f27877 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: Mike Percy Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] KUDU-1533 Spark Kudu Rdd/Dataframe upsert
Dan Burkert has posted comments on this change.
Change subject: KUDU-1533 Spark Kudu Rdd/Dataframe upsert
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3871/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
File
java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala:
Line 61: new KuduRelation(tableName, kuduMaster)(sqlContext)
Should the upsert flag get passed here as well?
Line 78: override def createRelation(sqlContext: SQLContext, mode: SaveMode,
How would you feel about removing the method and not implementing
CreatableRelationProvider? As far as I can tell we are not, and can not,
implement it correctly, since the main point of it is to create a new table
when it doesn't exist.
Line 94: case Overwrite => kuduRelation.insert(data, overwrite = true)
Throw Unsupported here as well for the reasons below.
Line 218: context.writeRows(data, tableName, overwrite, upsert)
Could you actually change this to throw a NotSupported exception when overwrite
= true? I think the Spark semantics are supposed to be that overwrite will
truncate the dataset then do inserts, which we can't yet support. Changing it
to throw will allow us to backwards compatibly support it later when we have
lightweight table truncation.
http://gerrit.cloudera.org:8080/#/c/3871/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
File
java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala:
Line 132: def writeRows(data: DataFrame, tableName: String, overwrite:
Boolean, upsert: Boolean) {
Instead of writeRows taking two booleans, could you split this into insertRows,
updateRows, and upsertRows? I think that will be a lot more clear. The
boolean was already really confusing.
Line 151: def writeRows(rows: Iterator[Row],
Same here. There shouldn't be too much code duplication if you pull out the
row building into a helper function.
Should this be private?
--
To view, visit http://gerrit.cloudera.org:8080/3871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley
Gerrit-Reviewer: Chris George
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR](gh-pages) www: Add Kudu talks to Community page
Mike Percy has uploaded a new change for review. http://gerrit.cloudera.org:8080/3877 Change subject: www: Add Kudu talks to Community page .. www: Add Kudu talks to Community page Change-Id: I26fde3dabd5f08ac656f286118704bec82f27877 --- M community.md 1 file changed, 9 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/3877/1 -- To view, visit http://gerrit.cloudera.org:8080/3877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I26fde3dabd5f08ac656f286118704bec82f27877 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: Mike Percy
[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()
Kudu Jenkins has posted comments on this change. Change subject: C++ client: deprecating KuduPartialRow::SetString() .. Patch Set 6: Build Started http://104.196.14.100/job/kudu-gerrit/2772/ -- To view, visit http://gerrit.cloudera.org:8080/3723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3723
to look at the new patch set (#6).
Change subject: C++ client: deprecating KuduPartialRow::SetString()
..
C++ client: deprecating KuduPartialRow::SetString()
KuduPartialRow::Set{String,Binary}() are marked as deprecated.
Use KuduPartialRow::Set{String,Binary}NoCopy() instead.
KuduPartialRow::SetString()/SetBinary() behavior is optimized
to make no copies of the passed data. However, a user of the API
might assume they are safe to use along with other setters as
SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, new methods are introduced instead:
KuduPartialRow::Set{String,Binary}NoCopy(). The new names
contain a hint about the optimized behavior of these methods.
An alternative approach might be to change behavior of
KuduPartialRow::Set{String,Binary}() and introduce only
KuduPartialRow::Set{String,Binary}NoCopy(), but there are
concerns about compatibility and performance regressions.
Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 179 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3723/6
--
To view, visit http://gerrit.cloudera.org:8080/3723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3868
to look at the new patch set (#5).
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
[KuduPartialRow::Set{Binary,String}()] copy input data
KuduPartialRow::SetBinary()/SetString() behavior was optimized
to omit copying of the passed data. However, a user of the API
might assume these methods are safe to use along with other setters
as SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, the behavior of these methods has been changed
to immediately copy the input data.
This is a safe modification, but it is not backward-compatible
in semver notation since it changes the semantics
of already existing methods. However, we don't bump API version
since Kudu is not 1.0 yet. As for the ABI, it remains compatible
with the prior versions: the existing client C++ code
will still compile and link.
This approach may add some performance regression issues for
already existing clients, but hopefully it is negligible for most
of the C++ clients around. As for the Impala-Kudu integration,
it seems current Impala code uses SetString() only in one place
at be/src/sec/kudu-table-sink.cc, and that can be addressed
separately.
An alternative approach might be to deprecate
KuduPartialRow::Set{Binary,String}() in favor of the newly introduced
KuduPartialRow::Set{Binary,String}NoCopy() methods. That approach
would spare us from performance regression worries and compatibility
issues. However, after some discussion, introducing the copying
behavior of KuduPartialRow::SetBinary()/SetString() methods
appeared to be more beneficial in the long run
from the usability perspective.
Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 165 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/3868/5
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Kudu Jenkins has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/2771/
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3868
to look at the new patch set (#4).
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
[KuduPartialRow::Set{Binary,String}()] copy input data
KuduPartialRow::SetBinary()/SetString() behavior was optimized
to omit copying of the passed data. However, a user of the API
might assume these methods are safe to use along with other setters
as SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, the behavior of these methods has been changed
to immediately copy the input data.
This is a safe modification, but it is not backward-compatible
in semver notation since it changes the semantics
of already existing methods. However, we don't bump API version
since Kudu is not 1.0 yet. As for the ABI, it remains compatible
with the prior versions: the existing client C++ code
will still compile and link.
This approach may add some performance regression issues for
already existing clients, but hopefully it is negligible for most
of the C++ clients around. As for the Impala-Kudu integration,
it seems current Impala code uses SetString() only in one place
at be/src/sec/kudu-table-sink.cc, and that can be addressed
separately.
An alternative approach might be to deprecate
KuduPartialRow::Set{Binary,String}() in favor of the newly introduced
KuduPartialRow::Set{Binary,String}NoCopy() methods. That approach
would spare us from performance regression worries and compatibility
issues. However, after some discussion, introducing the copying
behavior of KuduPartialRow::SetBinary()/SetString() methods
appeared to be more beneficial in the long run
from the usability perspective.
Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 163 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/3868/4
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Kudu Jenkins has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/2770/
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Mike Percy has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3868/3/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
Line 189: TEST_F(PartialRowTest, TestSetBinaryAndString) {
> Yes, it's a good idea to have a helper method for those. Will do.
Well, it's undefined behavior what happens to a freed string but depending on
ASAN seems fine to me. The ASAN build runs before each commit, and really all
the time.
Here is what happens with ASAN when you use freed memory:
1. take ptr to heap and store it
2. free that memory -> ASAN marks the memory region as poisoned
3. access the pointed-to memory -> ASAN intercepts the memory access and
crashes the test with a bunch of debugging information
Line 262: ASSERT_OK(row.SetStringNoCopy(2, src_str));
> SetXxxNoCopy() are the newly introduced methods, so I think it's nice to ha
ah, that's fair. I noticed that all the existing uses of SetString() were
converted to use SetStringNoCopy() in this patch so considered that general
test coverage, if not direct unit test coverage.
Line 276: EXPECT_EQ(reinterpret_cast(src_str.data()),
> I thought this part was derived from the interface constraints for the Slic
That's a reasonable argument, no worries, I'm not really against keeping this
test.
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Alexey Serbin has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 3:
(3 comments)
Will post the updated version soon.
http://gerrit.cloudera.org:8080/#/c/3868/3/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
Line 189: TEST_F(PartialRowTest, TestSetBinaryAndString) {
> TestSetBinaryAndString and TestSetBinaryAndStringCopy seem like mostly copy
Yes, it's a good idea to have a helper method for those. Will do.
As for the ASAN-related thing, I do not see how to get the desired behavior in
a manageable way. As I see it, there are two questions here:
1. It's guaranteed that the use-after-delete will be visible at non-ASAN build,
but we want this test to be reliably working under any type of build, right?
2. Even if we are targeting this for ASAN-builds only, is it guaranteed the
deleted memory will be filled with some predictable pattern which we can rely
on for current and future versions of the ASAN library?
Line 262: ASSERT_OK(row.SetStringNoCopy(2, src_str));
> It's up to you but personally I think NoCopy already has a lot of coverage
SetXxxNoCopy() are the newly introduced methods, so I think it's nice to have
at least some basic coverage for them.
Line 276: EXPECT_EQ(reinterpret_cast(src_str.data()),
> I think this just asserts that the implementation works a certain way (i.e.
I thought this part was derived from the interface constraints for the Slice
and PartialRow classes:
1. The PartialRow::Get{Binary,String}() methods return the stored data, and
they do not copy the data, as reflected in corresponding comments for those
methods in the partial_row.h file.
2. By definition, the Slice class is a wrapper around the data, and it's data()
method should return the pointer that corresponds to the source string's data
in this case. When we create a Slice object using Slice(const string& s)
constructor, the result object should refer to the data of the original string.
3. The PartialRow::Set{String,Binary}NoCopy() set the value for the specified
column, making no copies of the passed data.
Given 1, 2, and 3, we should expect slice.data() == str.data().
Is there a mistake in the chain above?
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Mike Percy has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3868/3/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:
Line 189: TEST_F(PartialRowTest, TestSetBinaryAndString) {
TestSetBinaryAndString and TestSetBinaryAndStringCopy seem like mostly
copy/paste to me, can we avoid that by extracting a helper method?
Also, it would be nice to do SetString(2, src_str) and then have src_str go out
of scope before continuing. That way we get ASAN to do the work for us.
Line 262: ASSERT_OK(row.SetStringNoCopy(2, src_str));
It's up to you but personally I think NoCopy already has a lot of coverage in
existing tests and is less important to add coverage for.
Line 276: EXPECT_EQ(reinterpret_cast(src_str.data()),
I think this just asserts that the implementation works a certain way (i.e.
slices pointing to the same buffer). Personally I don't find that these types
of tests add a lot of value because they are so brittle. However if you want to
include this check then I'm alright with it.
Usually I would rather use something a little higher level to assert that the
contract provided by the interface is respected. That's easy to do with the
Copy() version and hard to do with the NoCopy() version, I think.
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3723
to look at the new patch set (#5).
Change subject: C++ client: deprecating KuduPartialRow::SetString()
..
C++ client: deprecating KuduPartialRow::SetString()
KuduPartialRow::Set{String,Binary}() are marked as deprecated.
Use KuduPartialRow::Set{String,Binary}NoCopy() instead.
KuduPartialRow::SetString()/SetBinary() behavior is optimized
to make no copies of the passed data. However, a user of the API
might assume they are safe to use along with other setters as
SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, new methods are introduced instead:
KuduPartialRow::Set{String,Binary}NoCopy(). The new names
contain a hint about the optimized behavior of these methods.
An alternative approach might be to change behavior of
KuduPartialRow::Set{String,Binary}() and introduce only
KuduPartialRow::Set{String,Binary}NoCopy(), but there are
concerns about compatibility and performance regressions.
Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 202 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3723/5
--
To view, visit http://gerrit.cloudera.org:8080/3723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
[kudu-CR] C++ client: deprecating KuduPartialRow::SetString()
Kudu Jenkins has posted comments on this change. Change subject: C++ client: deprecating KuduPartialRow::SetString() .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/2769/ -- To view, visit http://gerrit.cloudera.org:8080/3723 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I348b0b9437b8d7928e3b607a0e0610d8d0c58f7c Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] cfile-test: some test micro-optimization to avoid timeouts
Todd Lipcon has submitted this change and it was merged. Change subject: cfile-test: some test micro-optimization to avoid timeouts .. cfile-test: some test micro-optimization to avoid timeouts It turns out that SCOPED_TRACE is relatively slow. Removing some SCOPED_TRACE in hot parts of the test speeds some of the test cases up by almost 2x in ASAN builds on my box. Given that these tests are fairly deterministic given a seed, and have been stable for a long time, leaving the SCOPED_TRACEs in has limited value. I only removed those that are in hot paths rather than removing all SCOPED_TRACEs. I also optimized the "large strings" test to avoid using a large amount of padding via the StringPrintf argument. The new implementation seems to run almost twice as fast on my machine. Comparing the 'CacheTypes/TestCFileBothCacheTypes.TestReadWriteLargeStrings/0' in fast-test ASAN mode before and after: Before: real0m39.710s user0m28.808s sys 0m3.828s After: real0m21.274s user0m9.824s sys 0m3.832s Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Reviewed-on: http://gerrit.cloudera.org:8080/3875 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/cfile/cfile-test-base.h M src/kudu/cfile/cfile-test.cc 2 files changed, 37 insertions(+), 22 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [client][gutil] introduced ATTRIBUTE DEPRECATED
Mike Percy has posted comments on this change. Change subject: [client][gutil] introduced ATTRIBUTE_DEPRECATED .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0d3c48fe8b5b599126ff45728d0b8da30ffe082 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [client][gutil] introduced ATTRIBUTE DEPRECATED
Mike Percy has submitted this change and it was merged.
Change subject: [client][gutil] introduced ATTRIBUTE_DEPRECATED
..
[client][gutil] introduced ATTRIBUTE_DEPRECATED
The ATTRIBUTE_DEPRECATED macro is to mark deprecated methods.
An example of usage:
void foo() ATTRIBUTE_DEPRECATED("use bar() instead");
Change-Id: Ib0d3c48fe8b5b599126ff45728d0b8da30ffe082
Reviewed-on: http://gerrit.cloudera.org:8080/3874
Reviewed-by: Todd Lipcon
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy
---
M src/kudu/client/stubs.h
M src/kudu/gutil/port.h
2 files changed, 24 insertions(+), 0 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3874
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib0d3c48fe8b5b599126ff45728d0b8da30ffe082
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Kudu Jenkins has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2768/
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3868
to look at the new patch set (#3).
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
[KuduPartialRow::Set{Binary,String}()] copy input data
KuduPartialRow::SetBinary()/SetString() behavior was optimized
to omit copying of the passed data. However, a user of the API
might assume these methods are safe to use along with other setters
as SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, the behavior of these methods has been changed
to immediately copy the input data.
This is a safe modification, but it is not backward-compatible
in semver notation since it changes the semantics
of already existing methods. However, we don't bump API version
since Kudu is not 1.0 yet. As for the ABI, it remains compatible
with the prior versions: the existing client C++ code
will still compile and link.
This approach may add some performance regression issues for
already existing clients, but hopefully it is negligible for most
of the C++ clients around. As for the Impala-Kudu integration,
it seems current Impala code uses SetString() only in one place
at be/src/sec/kudu-table-sink.cc, and that can be addressed
separately.
An alternative approach might be to deprecate
KuduPartialRow::Set{Binary,String}() in favor of the newly introduced
KuduPartialRow::Set{Binary,String}NoCopy() methods. That approach
would spare us from performance regression worries and compatibility
issues. However, after some discussion, introducing the copying
behavior of KuduPartialRow::SetBinary()/SetString() methods
appeared to be more beneficial in the long run
from the usability perspective.
Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
---
M src/kudu/client/predicate-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
12 files changed, 189 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/3868/3
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
[kudu-CR] cfile-test: some test micro-optimization to avoid timeouts
Adar Dembo has posted comments on this change. Change subject: cfile-test: some test micro-optimization to avoid timeouts .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] cfile-test: some test micro-optimization to avoid timeouts
Kudu Jenkins has posted comments on this change. Change subject: cfile-test: some test micro-optimization to avoid timeouts .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2767/ -- To view, visit http://gerrit.cloudera.org:8080/3875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] cfile-test: some test micro-optimization to avoid timeouts
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3875 to look at the new patch set (#2). Change subject: cfile-test: some test micro-optimization to avoid timeouts .. cfile-test: some test micro-optimization to avoid timeouts It turns out that SCOPED_TRACE is relatively slow. Removing some SCOPED_TRACE in hot parts of the test speeds some of the test cases up by almost 2x in ASAN builds on my box. Given that these tests are fairly deterministic given a seed, and have been stable for a long time, leaving the SCOPED_TRACEs in has limited value. I only removed those that are in hot paths rather than removing all SCOPED_TRACEs. I also optimized the "large strings" test to avoid using a large amount of padding via the StringPrintf argument. The new implementation seems to run almost twice as fast on my machine. Comparing the 'CacheTypes/TestCFileBothCacheTypes.TestReadWriteLargeStrings/0' in fast-test ASAN mode before and after: Before: real0m39.710s user0m28.808s sys 0m3.828s After: real0m21.274s user0m9.824s sys 0m3.832s Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 --- M src/kudu/cfile/cfile-test-base.h M src/kudu/cfile/cfile-test.cc 2 files changed, 37 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/3875/2 -- To view, visit http://gerrit.cloudera.org:8080/3875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Support add/remove partition
Adar Dembo has posted comments on this change.
Change subject: [java client] Support add/remove partition
..
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java:
Line 118:* other or any existing range partitions (unless the existing
range partitions are dropped as
> But it is transactional in the sense that either all operations are applied
OK, fair. Truth be told, the wait() functionality that's built into the
KuduTableAlterer (C++ client, but I imagine there's an equivalent in Java too)
can guarantee that when the "transaction" finishes, all tablets have been
altered.
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
PS1, Line 1380: if (oldRt != null) {
: // someone beat us to it
: continue;
> Done
I think you missed this.
http://gerrit.cloudera.org:8080/#/c/3854/2/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
Line 349: public Deferred alterTable(String name, AlterTableOptions ato) {
Why drop the generic type?
Line 356: // Clear the caches so the new partition is immediately visible.
Nit: update this comment.
Line 1350: // already be present
Nit: terminate with period.
Line 1361: // Build of the list of discovered remote tablet instances. If
we have
Nit: "Build the list..."
Line 1362: // already discovered the tablet, it's locations are refreshed.
Nit: "its"
Line 1365: // Early creating the tablet so that it parses out the pb
Nit: terminate with period.
Line 1370: RemoteTablet currentTablet = tablet2client.get(tabletId);
As we discussed, it looks like there's an opportunity to get rid of
tablet2client entirely, provided 1) we replace this lookup with a lookup into
the main table locations cache, 2) adjust the locking so that there's no chance
of a race between this lookup and the innards of cacheTabletLocations().
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java:
Line 170: if (floorEntry != null &&
> Line 167 is limiting the removed set to the suffix after the lower bound, w
I see. I missed that this line acts on overlappingEntries, not entries.
Line 200: public static class Entry {
> If you insist I can change it, but I think DeadlineTrackers are overkill fo
I don't feel strongly, no.
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java:
Line 282: String tableName = name.getMethodName() +
System.currentTimeMillis();
> Done
Thanks. Could you fix it for the other tests in this file too?
--
To view, visit http://gerrit.cloudera.org:8080/3854
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] cfile-test: some test micro-optimization to avoid timeouts
Adar Dembo has posted comments on this change. Change subject: cfile-test: some test micro-optimization to avoid timeouts .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/3875/1//COMMIT_MSG Commit Message: PS1, Line 10: is hot parts of the test Nit: "is" makes the sentence read in a strange way. -- To view, visit http://gerrit.cloudera.org:8080/3875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java client] Support add/remove partition
Dan Burkert has posted comments on this change. Change subject: [java client] Support add/remove partition .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: PS1, Line 360: tableLocations.clear(); : tablet2client.clear(); > What's the intuition behind clearing these two but not client2tablet? This only needs to clear the locations cache. I changed it to just that. -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [java client] Support add/remove partition
Dan Burkert has posted comments on this change.
Change subject: [java client] Support add/remove partition
..
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java:
Line 19: import com.google.common.base.Preconditions;
> This doesn't make Guava part of the API itself, right? This will be our sha
Correct; it's only an issue if we take a Guava type as an argument, or return a
Guava type.
Line 37: /**
> Hmm, can we make this private?
Done
Line 118:* other or any existing range partitions (unless the existing
range partitions are dropped as
> Nit: I get that this is copied from the C++ client, but we should probably
But it is transactional in the sense that either all operations are applied, or
they are all rejected atomically. We sometimes overload operation to mean a
specific step, and sometimes the whole set of steps.
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
PS1, Line 697: // called via a callback that won't care about the
returned Deferred.
: request.errback(e);
> There's no danger of a code path that would "double count" the error by vir
I'm not really sure what it means to count an error? But my understanding is
that either callers are using the deferred, or they are using the errback chain
of the request. Why we use one in some cases and the other in others is a
mystery to me.
PS1, Line 1343:List locations,
:long ttl) throws NonRecoverableException
> This comment needs to be updated.
Done
Line 1366: RemoteTablet rt = createTabletFromPb(tableId, tabletPb);
> Why this change? Doesn't it contradict this block's comment?
Yah, the way this works has changed. We need to send the existing tablets to
the table locations cache in order to update the TTL, and use it to discover
non-covered ranges. I've updated the comment.
PS1, Line 1380: if (oldRt != null) {
: // someone beat us to it
: continue;
> Needs to be updated. To be honest, all the comments in this method could st
Done
PS1, Line 1395: locationsCache.cacheTabletLocations(tablets,
requestPartitionKey, ttl);
: }
:
: RemoteTablet createTabletFromPb(String tableId,
Master.TabletLocationsPB tabletPb) {
: Partition partition =
ProtobufHelper.pbToPartition(tabletPb.getPartition());
: S
> Update this.
Done
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java:
Line 77:*
> As written, each new entry is likely to have a slightly different deadline,
Done
PS1, Line 99: List newEntries = new ArrayList<>();
:
> Nit: indentation.
Done
Line 170: if (floorEntry != null &&
> If we're executing this line, why bother executing L167-168?
Line 167 is limiting the removed set to the suffix after the lower bound, while
this line is limiting the removed set the the prefix before the upper bound.
Together, they limit the remove set between the two bounds.
Line 200: public static class Entry {
> Maybe we can track these via DeadlineTrackers?
If you insist I can change it, but I think DeadlineTrackers are overkill for
this. This deadline doesn't need to be resettable, and I have a hard time
figuring out the DeadlineTracker API to begin with. Why the heck does it need
a Stopwatch? Why is it resettable?
Line 246: return tablet == null ? lowerBoundPartitionKey :
tablet.getPartition().getPartitionKeyStart();
> Should we prevent this value from becoming negative (i.e. return zero if th
I've changed this to be a private method, since it's only used for printing. I
think it may be useful in some circumstances to see how expired an entry is.
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> Missing copyright statement.
Done
PS1, Line 32:
:
> That's not true if I pass something into 'bounds', right?
Done
http://gerrit.cloudera.org:8080/#/c/3854/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java:
Line 282: String tableName = name.getMethodName() +
System.currentTimeMillis();
> N
[kudu-CR] [java client] Support add/remove partition
Kudu Jenkins has posted comments on this change. Change subject: [java client] Support add/remove partition .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2766/ -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java client] Support add/remove partition
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3854 to look at the new patch set (#2). Change subject: [java client] Support add/remove partition .. [java client] Support add/remove partition This also sneaks a fix into catalog manager to change the status type when rejecting invalid alter table partitioning operations. Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde --- M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java D java/kudu-client/src/main/java/org/apache/kudu/client/NonCoveredRangeCache.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java A java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.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/TestAsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java M src/kudu/master/catalog_manager.cc 13 files changed, 877 insertions(+), 289 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/3854/2 -- To view, visit http://gerrit.cloudera.org:8080/3854 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If5da5f0d3e677e62256a8a6d2107093bbda44cde Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [client][gutil] introduced ATTRIBUTE DEPRECATED
Todd Lipcon has posted comments on this change. Change subject: [client][gutil] introduced ATTRIBUTE_DEPRECATED .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0d3c48fe8b5b599126ff45728d0b8da30ffe082 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] cfile-test: some test micro-optimization to avoid timeouts
Kudu Jenkins has posted comments on this change. Change subject: cfile-test: some test micro-optimization to avoid timeouts .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2765/ -- To view, visit http://gerrit.cloudera.org:8080/3875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] cfile-test: some test micro-optimization to avoid timeouts
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/3875 to review the following change. Change subject: cfile-test: some test micro-optimization to avoid timeouts .. cfile-test: some test micro-optimization to avoid timeouts It turns out that SCOPED_TRACE is relatively slow. Removing some SCOPED_TRACE is hot parts of the test speed some of the test cases up by almost 2x in ASAN builds on my box. Given that these tests are fairly deterministic given a seed, and have been stable for a long time, leaving the SCOPED_TRACEs in has limited value. I only removed those that are in hot paths rather than removing all SCOPED_TRACEs. I also optimized the "large strings" test to avoid using a large amount of padding via the StringPrintf argument. The new implementation seems to run almost twice as fast on my machine. Comparing the 'CacheTypes/TestCFileBothCacheTypes.TestReadWriteLargeStrings/0' in fast-test ASAN mode before and after: Before: real0m39.710s user0m28.808s sys 0m3.828s After: real0m21.274s user0m9.824s sys 0m3.832s Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 --- M src/kudu/cfile/cfile-test-base.h M src/kudu/cfile/cfile-test.cc 2 files changed, 37 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/3875/1 -- To view, visit http://gerrit.cloudera.org:8080/3875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I951f4ef47c5275ea9743b1be0ff74374498192b1 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo
[kudu-CR] [client][gutil] introduced ATTRIBUTE DEPRECATED
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3874
Change subject: [client][gutil] introduced ATTRIBUTE_DEPRECATED
..
[client][gutil] introduced ATTRIBUTE_DEPRECATED
The ATTRIBUTE_DEPRECATED macro is to mark deprecated methods.
An example of usage:
void foo() ATTRIBUTE_DEPRECATED("use bar() instead");
Change-Id: Ib0d3c48fe8b5b599126ff45728d0b8da30ffe082
---
M src/kudu/client/stubs.h
M src/kudu/gutil/port.h
2 files changed, 24 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/3874/1
--
To view, visit http://gerrit.cloudera.org:8080/3874
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib0d3c48fe8b5b599126ff45728d0b8da30ffe082
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
[kudu-CR] [client][gutil] introduced ATTRIBUTE DEPRECATED
Kudu Jenkins has posted comments on this change. Change subject: [client][gutil] introduced ATTRIBUTE_DEPRECATED .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2764/ -- To view, visit http://gerrit.cloudera.org:8080/3874 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0d3c48fe8b5b599126ff45728d0b8da30ffe082 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Alexey Serbin has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 2:
(3 comments)
> (3 comments)
>
> Can we add basic test coverage for SetString() and SetBinary()?
Yes, why not? Will do.
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/client/stubs.h
File src/kudu/client/stubs.h:
Line 66: // For deprecated functions or variables, generate a warning at usage
sites.
> If you want to bring in this helper as part of this patch, just mention it
OK, let me make a separate changelist bringing in this ATTRIBUTE_DEPRECATED
macro. I think it will be better that way.
As for presence of this macro in both stubs.h and port.h: as I understand,
port.h is for usage within the project, and stubs.h is distributed along with
our client C++ library and other files necessary for development of client-side
applications using Kudu C++ client API. So, that's why two separate files.
You can find that other macros which are used both throughout the project
internally and by the client C++ API utilize the same approach: e.g., check for
ATTRIBUTE_UNUSED.
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
Line 277: return SetSliceCopy >(col_name, val);
> Can we just delegate these calls to SetBinaryCopy() so it's clear it's just
That's was my initial approach (you can see it in the earlier versions). Then
I realized there is some extra-call, and decided to switch to this version.
OK, I'll bring the older version back.
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
Line 92: // Sets the binary/binary value, copying 'val' data immediately.
Essentially,
> typo: binary/binary
Done
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Mike Percy has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 2:
(3 comments)
Can we add basic test coverage for SetString() and SetBinary()?
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/client/stubs.h
File src/kudu/client/stubs.h:
Line 66: // For deprecated functions or variables, generate a warning at usage
sites.
If you want to bring in this helper as part of this patch, just mention it in
the commit message.
Also, I wonder why this is defined both in port.h and here... maybe we should
not define it here if it's not used in the client.
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/common/partial_row.cc
File src/kudu/common/partial_row.cc:
Line 277: return SetSliceCopy >(col_name, val);
Can we just delegate these calls to SetBinaryCopy() so it's clear it's just an
alias? Or vice versa?
http://gerrit.cloudera.org:8080/#/c/3868/2/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
Line 92: // Sets the binary/binary value, copying 'val' data immediately.
Essentially,
typo: binary/binary
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata
Mike Percy has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/3823/6/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: Line 211: peer->tablet_metadata()->schema()); Note: schema_ is also modified in TabletMetadata::LoadFromSuperBlock() -- To view, visit http://gerrit.cloudera.org:8080/3823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Kudu Jenkins has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/2763/
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3868
to look at the new patch set (#2).
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
[KuduPartialRow::Set{Binary,String}()] copy input data
KuduPartialRow::SetBinary()/SetString() behavior was optimized
to omit copying of the passed data. However, a user of the API
might assume these methods are safe to use along with other setters
as SetInt32, SetDouble, etc. where the string or binary data
goes out of scope (or deallocated) before AppendToPB() is called.
To play safe, the behavior of these methods has been changed
to immediately copy the input data.
This is a safe modification, but it is not backward-compatible
in semver notation since it changes the semantics
of already existing methods. However, we don't bump API version
since Kudu is not 1.0 yet. As for the ABI, it remains compatible
with the prior versions: the existing client C++ code
will still compile and link.
This approach may add some performance regression issues for
already existing clients, but hopefully it is negligible for most
of the C++ clients around. As for the Impala-Kudu integration,
it seems current Impala code uses SetString() only in one place
at be/src/sec/kudu-table-sink.cc, and that can be addressed
separately.
An alternative approach might be to deprecate
KuduPartialRow::Set{Binary,String}() in favor of the newly introduced
KuduPartialRow::Set{Binary,String}NoCopy() methods. That approach
would spare us from performance regression worries and compatibility
issues. However, after some discussion, introducing the copying
behavior of KuduPartialRow::SetBinary()/SetString() methods
appeared to be more beneficial in the long run
from the usability perspective.
Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/stubs.h
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/row_operations-test.cc
M src/kudu/gutil/port.h
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/write_throttling-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tserver/tablet_copy_session-test.cc
14 files changed, 110 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/3868/2
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up
Kudu Jenkins has posted comments on this change. Change subject: KUDU-763 consensus queue metrics on followers are messed up .. Patch Set 8: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2762/ -- To view, visit http://gerrit.cloudera.org:8080/3501 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] [KuduPartialRow::Set{Binary,String}()] copy input data
Alexey Serbin has posted comments on this change.
Change subject: [KuduPartialRow::Set{Binary,String}()] copy input data
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3868/1//COMMIT_MSG
Commit Message:
Line 17: This is safe, but an incompatible change, but we don't bump
> Code will still link / compile so this isn't API or ABI compatible. It's ju
Done
http://gerrit.cloudera.org:8080/#/c/3868/1/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
Line 108: // must remain valid until data for corresponding RPC calls are
generated.
> Isn't the rule that the data must remain valid until the corresponding RPC
Thank you for bringing in this question -- I decided to look around to clarify
on this.
As I see from the code, the slice's data is encoded into PB packet before
issuing corresponding RPC call (see RowOperationsPBEncoder::Add() in
row_operations.cc). So, if talking about when it's save to release the data
which is necessary to make corresponding RPC call, then it's about waiting for
data to be encoded into the PB packet, not sending the request and awaiting for
completion.
However, if an error happened, then the error information returned by
KuduSession::GetPendingErrors() refers to the original KuduWriteOperation
object, which in its turn refers to KuduPartialRow object. The KuduPartialRow
object refers to its slice objects, and the corresponding slice objects refers
to our binary/string data. So, if an error happened and somebody is to analyze
the error, the slice data should be kept alive till the end of the call.
In other words, you are right -- to be sure to have safe access the data
corresponding to errors, if any, it's necessary to keep that data around until
completion of the corresponding RPC calls.
I will update the comment.
--
To view, visit http://gerrit.cloudera.org:8080/3868
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I764339a0e3ffbf6abd5372e682c9fa1792bdd52b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1533 Spark Kudu Rdd/Dataframe upsert
Will Berkeley has uploaded a new change for review. http://gerrit.cloudera.org:8080/3871 Change subject: KUDU-1533 Spark Kudu Rdd/Dataframe upsert .. KUDU-1533 Spark Kudu Rdd/Dataframe upsert This patch adds support for upsert in Kudu dataframes. The SaveMode parameter to createRelation does not map to Kudu very well: Append is fine, but Overwrite is supposed to mean truncate and then insert. Kudu does not (currently) support truncation of tables, but it does support updates and upserts, so Overwrite is taken to mean "upsert new rows". Circumventing the limitations of the datasource API, users can still restore the old behavior of mode Overwrite (update, no insert) by setting kudu.upsert = false in the options. I think users prefer to have upsert semantics by default. Additionally, Ignore was previously given the same meaning as Append, contrary to its intended meaning. It's been re-categorized as unsupported along with ErrorIfExisting. Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822 --- M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala 3 files changed, 62 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/3871/1 -- To view, visit http://gerrit.cloudera.org:8080/3871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley
[kudu-CR] KUDU-1533 Spark Kudu Rdd/Dataframe upsert
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1533 Spark Kudu Rdd/Dataframe upsert .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2761/ -- To view, visit http://gerrit.cloudera.org:8080/3871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR](gh-pages) Simple typo fix in faq: 'arge' => 'large'
Todd Lipcon has posted comments on this change. Change subject: Simple typo fix in faq: 'arge' => 'large' .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/3869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I675d770f8fa484cf6fff7a7006f0dbd68eb51517 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: Brock Noland Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR](gh-pages) Simple typo fix in faq: 'arge' => 'large'
Todd Lipcon has submitted this change and it was merged. Change subject: Simple typo fix in faq: 'arge' => 'large' .. Simple typo fix in faq: 'arge' => 'large' Change-Id: I675d770f8fa484cf6fff7a7006f0dbd68eb51517 Reviewed-on: http://gerrit.cloudera.org:8080/3869 Reviewed-by: Todd Lipcon Tested-by: Todd Lipcon --- M faq.md 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Todd Lipcon: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/3869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I675d770f8fa484cf6fff7a7006f0dbd68eb51517 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: Brock Noland Gerrit-Reviewer: Todd Lipcon
[kudu-CR](gh-pages) Simple typo fix in faq: 'arge' => 'large'
Todd Lipcon has posted comments on this change. Change subject: Simple typo fix in faq: 'arge' => 'large' .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I675d770f8fa484cf6fff7a7006f0dbd68eb51517 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: Brock Noland Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rev 2
Dinesh Bhat has abandoned this change. Change subject: rev 2 .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/3870 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I779f00d3562484fb79dd7ff822d9c4b8939831c2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata
Dinesh Bhat has posted comments on this change.
Change subject: KUDU-1500: Data race in
RaftConsensusITest.TestCorruptReplicaMetadata
..
Patch Set 6:
(1 comment)
TFTR MIke, this is still WIP, I uploaded one missing piece from my previous
patchset to confirm jenkins test suite pass. I will upload another patchset
today.
http://gerrit.cloudera.org:8080/#/c/3823/4/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:
Line 142: const PartitionSchema partition_schema_locked() const {
> I don't think this is needed anymore if we make PartitionSchema::PartitionD
I think I had missed out one important callsite in ListTablets(), while
cleaning up the changes. I am inclined to keep this routine and
PartitionDebugString as it is since this routine is called by 2 other callers
now. This is still WIP as I uploaded the patch only to see if kudu jenkins test
suite is happy,
--
To view, visit http://gerrit.cloudera.org:8080/3823
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3823 to look at the new patch set (#6). Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata .. KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata The test intends to corrupt the metadata of one of the tserver tablets. While the cluster is in the process of resurrecting the failed tserver, the partition schema on that tserver is accessed in an unguarded manner from a thread servicing ListTablets API. The proposed fix here accesses the metadata partition resources via copy by value mechanism now in slow paths, where the copy itself happens under the same lock which makes the metadata resurrection operation idempotent. Fast paths are untouched since the parition schema is bound to be accessed in fast paths only after tablet metadata is fully resurrected and tablet is in RUNNING state. Testing: Passed about 2000 iteration of the failing test raft_consensus-itest.TestCorruptReplicaMetadata Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019 --- M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_bootstrap.h M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/tablet_peer.cc M src/kudu/tools/fs_tool.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver-path-handlers.cc 8 files changed, 20 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/3823/6 -- To view, visit http://gerrit.cloudera.org:8080/3823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata .. Patch Set 6: Build Started http://104.196.14.100/job/kudu-gerrit/2760/ -- To view, visit http://gerrit.cloudera.org:8080/3823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rev 2
Dinesh Bhat has uploaded a new change for review. http://gerrit.cloudera.org:8080/3870 Change subject: rev 2 .. rev 2 Change-Id: I779f00d3562484fb79dd7ff822d9c4b8939831c2 --- M src/kudu/tserver/tablet_service.cc 1 file changed, 2 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/3870/1 -- To view, visit http://gerrit.cloudera.org:8080/3870 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I779f00d3562484fb79dd7ff822d9c4b8939831c2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat
[kudu-CR] KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/2758/ -- To view, visit http://gerrit.cloudera.org:8080/3823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rev 2
Kudu Jenkins has posted comments on this change. Change subject: rev 2 .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2759/ -- To view, visit http://gerrit.cloudera.org:8080/3870 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I779f00d3562484fb79dd7ff822d9c4b8939831c2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
