[kudu-CR] KUDU-2511 fix for SingleReplicasStayOrMove scenario
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11046 ) Change subject: KUDU-2511 fix for SingleReplicasStayOrMove scenario .. Patch Set 1: Verified+1 Unrelated flake in: org.apache.kudu.backup.TestKuduBackup.Random Backup and Restore -- To view, visit http://gerrit.cloudera.org:8080/11046 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibfa74d5a78e89bf7ff1a0e914384999466460145 Gerrit-Change-Number: 11046 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Comment-Date: Wed, 25 Jul 2018 05:11:27 + Gerrit-HasComments: No
[kudu-CR] KUDU-2511 fix for SingleReplicasStayOrMove scenario
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11046 ) Change subject: KUDU-2511 fix for SingleReplicasStayOrMove scenario .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11046 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ibfa74d5a78e89bf7ff1a0e914384999466460145 Gerrit-Change-Number: 11046 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] memrowset: support iteration with is deleted virtual column
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10990 to look at the new patch set (#7). Change subject: memrowset: support iteration with is_deleted virtual column .. memrowset: support iteration with is_deleted virtual column This patch rounds out the MemRowSet changes for incremental backups. Taken together, it is now possible to iterate on a specific time range and to learn which rows were deleted during that time range. Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 --- M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h 3 files changed, 131 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/10990/7 -- To view, visit http://gerrit.cloudera.org:8080/10990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 Gerrit-Change-Number: 10990 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] schema: add is deleted virtual column
Hello Tidy Bot, Mike Percy, Dan Burkert, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10968 to look at the new patch set (#9). Change subject: schema: add is_deleted virtual column .. schema: add is_deleted virtual column This patch introduces a very basic concept of a "virtual column", defined as a static column with a name, type, and defaults. A Kudu subsystem that wishes to interact with a virtual column needs to first figure out if the projection includes it (via ColumnSchema::Equals()). When projected, the virtual column's data will be entirely default; it's the subsystem's responsibility to fill in something meaningful afterwards. This first cut is hardly robust. It remains to be seen how usable this bare-bones abstraction will be; so far it's sufficient for the MemRowSet. Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e --- M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc 4 files changed, 104 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/9 -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2511 fix for SingleReplicasStayOrMove scenario
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11046 Change subject: KUDU-2511 fix for SingleReplicasStayOrMove scenario .. KUDU-2511 fix for SingleReplicasStayOrMove scenario The RebalancerAndSingleReplicaTablets.SingleReplicasStayOrMove scenario had a rare flake: sometimes after the restart of tablet servers that were shutdown to create unbalanced tablet distribution, ksck result would be inconsistent because it fetched information on tablet servers and tables, but failed to fetch consensus info for corresponding tablets. In that case, the input for the rebalancing algorithm would contain no table/tablet information, so the rebalancer would not move a single replica. The issue with the consistency of the ksck results will be addressed in a separate patch. Change-Id: Ibfa74d5a78e89bf7ff1a0e914384999466460145 --- M src/kudu/tools/kudu-admin-test.cc 1 file changed, 19 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/11046/1 -- To view, visit http://gerrit.cloudera.org:8080/11046 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibfa74d5a78e89bf7ff1a0e914384999466460145 Gerrit-Change-Number: 11046 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] memrowset: support iteration with include deleted rows
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10929 ) Change subject: memrowset: support iteration with include_deleted_rows .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/10929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie018043518b437ecc719cf9f87b2c5eea560c9a1 Gerrit-Change-Number: 10929 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Jul 2018 02:47:21 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with snap to exclude
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 8: Code-Review+1 much easier for me to follow now, thanks! -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Jul 2018 02:46:10 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with is deleted virtual column
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10990 ) Change subject: memrowset: support iteration with is_deleted virtual column .. Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset-test.cc@737 PS5, Line 737: } > same comment about testing the case with reinserts, just for good measure Done http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.h@594 PS5, Line 594: size_t projection_vc_is_deleted_idx_; > shouldn't this be int? I think kColumnNotFound is -1 Oops. Turns out that didn't cause any problems because of how we're using projection_vc_is_deleted_idx_, but good to fix regardless. http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.cc@404 PS5, Line 404: projection > if projection is a schema, could we just use find_column here? This is a somewhat stronger search (in that the projection's column must match kVirtualColumnIsDeleted by type, encoding, compression, etc.) but that probably doesn't matter. http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.cc@562 PS5, Line 562: *reinterpret_cast(dst_row.mutable_cell_ptr(projection_vc_is_deleted_idx_)) = true; > Can you use UnalignedStore here instead? It compiles to the same thing but Done -- To view, visit http://gerrit.cloudera.org:8080/10990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 Gerrit-Change-Number: 10990 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Jul 2018 02:30:45 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10926 to look at the new patch set (#8). Change subject: memrowset: support iteration with snap_to_exclude .. memrowset: support iteration with snap_to_exclude Following on the commit that introduced the snap_to_exclude iterator option, this commit modifies the MemRowSet iterator to take snap_to_exclude into consideration (if it is set) when determining which rows and mutations are relevant to an iterator. I've also pulled the "exceeded the iterator's scan bounds" short-circuit out of the MVCC checks. If it were true for a row excluded by MVCC, it'd be true for all subsequent rows too. Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce --- M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 5 files changed, 173 insertions(+), 40 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/10926/8 -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: support iteration with include deleted rows
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10929 ) Change subject: memrowset: support iteration with include_deleted_rows .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset-test.cc@585 PS5, Line 585: INSTANTIATE_TEST_CASE_P(MrsConfigurations, ParameterizedTestMemRowSet, : ::testing::Bool()); > mind naming this parameterization something more useful to indicate what th Done http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset-test.cc@665 PS5, Line 665: ASSERT_OK(InsertRow(mrs.get(), "row 1", 0)); > I think its' worth adding a third row which has been inserted, deleted, and Done http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset.h@557 PS5, Line 557: // The 'was_deleted' parameter will be set to true if the last mutation walked > ah, this is starting to look a bit closer to my proposal to add an enum out Technically they're all possible, but I think this is still cleaner with your enum suggestion from the first patch. Take a look. -- To view, visit http://gerrit.cloudera.org:8080/10929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie018043518b437ecc719cf9f87b2c5eea560c9a1 Gerrit-Change-Number: 10929 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Jul 2018 02:30:33 + Gerrit-HasComments: Yes
[kudu-CR] schema: add is deleted virtual column
Hello Mike Percy, Dan Burkert, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10968 to look at the new patch set (#8). Change subject: schema: add is_deleted virtual column .. schema: add is_deleted virtual column This patch introduces a very basic concept of a "virtual column", defined as a static column with a name, type, and defaults. A Kudu subsystem that wishes to interact with a virtual column needs to first figure out if the projection includes it (via ColumnSchema::Equals()). When projected, the virtual column's data will be entirely default; it's the subsystem's responsibility to fill in something meaningful afterwards. This first cut is hardly robust. It remains to be seen how usable this bare-bones abstraction will be; so far it's sufficient for the MemRowSet. Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e --- M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test.cc 4 files changed, 103 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/10968/8 -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/10926 )
Change subject: memrowset: support iteration with snap_to_exclude
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@514
PS7, Line 514: bool unset_in_sel_vector
> this name is making me a little confused, because it can change again after
Yeah, I agree that mutating unset_in_sel_vector inside
ApplyMutationsToProjectedRow makes this harder to understand.
I think a descriptive enum as an out parameter is a nice touch. I'll make the
change here knowing that the net result is more information than necessary for
this patch, but useful for the next one.
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@517
PS7, Line 517: if (has_upper_bound() && out_of_bounds(k)) {
: state_ = kFinished;
: break;
> I wonder if this actually belongs outside the if condition? Even if we excl
Makes sense. I'll pull this check out of the outer if.
--
To view, visit http://gerrit.cloudera.org:8080/10926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce
Gerrit-Change-Number: 10926
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 25 Jul 2018 02:30:30 +
Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with include deleted rows
Hello Mike Percy, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10929 to look at the new patch set (#6). Change subject: memrowset: support iteration with include_deleted_rows .. memrowset: support iteration with include_deleted_rows This is another piece in the incremental backup puzzle. The idea is that when taking an incremental backup, the scan will include rows that have been deleted. The backup will figure out which rows were deleted by including a special "is deleted" virtual column in the projection. This commit introduces a new iterator option that can be used to include deleted rows, and uses it in the MemRowSet. Change-Id: Ie018043518b437ecc719cf9f87b2c5eea560c9a1 --- M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h 4 files changed, 80 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/10929/6 -- To view, visit http://gerrit.cloudera.org:8080/10929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie018043518b437ecc719cf9f87b2c5eea560c9a1 Gerrit-Change-Number: 10929 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: support iteration with is deleted virtual column
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10990 to look at the new patch set (#6). Change subject: memrowset: support iteration with is_deleted virtual column .. memrowset: support iteration with is_deleted virtual column This patch rounds out the MemRowSet changes for incremental backups. Taken together, it is now possible to iterate on a specific time range and to learn which rows were deleted during that time range. Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 --- M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc M src/kudu/tablet/memrowset.h 3 files changed, 131 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/10990/6 -- To view, visit http://gerrit.cloudera.org:8080/10990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 Gerrit-Change-Number: 10990 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Scala code formatting with Scalafmt
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 ) Change subject: Scala code formatting with Scalafmt .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11030/6/java/.scalafmt.conf File java/.scalafmt.conf: http://gerrit.cloudera.org:8080/#/c/11030/6/java/.scalafmt.conf@4 PS6, Line 4: docstrings=JavaDoc Maybe switch this back to ScalaDoc. I looks like a lot of comments were just shifted by 1 space. -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 6 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Wed, 25 Jul 2018 02:13:19 + Gerrit-HasComments: Yes
[kudu-CR] Scala code formatting with Scalafmt
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 ) Change subject: Scala code formatting with Scalafmt .. Patch Set 6: (1 comment) You could try wrapping in a no format comment as a workaround. http://gerrit.cloudera.org:8080/#/c/11030/6/java/.gitignore File java/.gitignore: http://gerrit.cloudera.org:8080/#/c/11030/6/java/.gitignore@39 PS6, Line 39: **/out > let me know if you want this as part of a different commit. I think it's fine. -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 6 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Wed, 25 Jul 2018 02:10:31 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2095 - Add scanner `keepAlive` RPC call to Java API
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7749 to look at the new patch set (#8). Change subject: KUDU-2095 - Add scanner `keepAlive` RPC call to Java API .. KUDU-2095 - Add scanner `keepAlive` RPC call to Java API - Adds keepAlive methods to sync and async clients - Methods return a (Deferred) Status object Where possible the behavior mimics the C++ client. Publicly accessible methods are available in AsyncKuduScanner and KuduScanner. A call to KeepAlive will return a Status object which will be OK if - The scanner has been initialized - The scanner is active and has rows left to return - The scanner is between tablets It will return a non-ok status when - The scanner has not yet been initialized - The scanner has no more rows Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713 --- A java/kudu-client/out/test/resources/log4j.properties A java/kudu-client/out/test/resources/test-key-and-cert.jks M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java 6 files changed, 212 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/7749/8 -- To view, visit http://gerrit.cloudera.org:8080/7749 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba647e7b5ed4ee8d223d387b6656d3fb02c41713 Gerrit-Change-Number: 7749 Gerrit-PatchSet: 8 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Tony Foerster
[kudu-CR] Scala code formatting with Scalafmt
Tony Foerster has posted comments on this change. (
http://gerrit.cloudera.org:8080/11030 )
Change subject: Scala code formatting with Scalafmt
..
Patch Set 6:
Hmm, Jenkins is failing but the same target (:kudu-spark:testScalafmt) passes
locally for me, it's failing here
14:48:06 Execution failed for task ':kudu-spark:testScalafmt'.
14:48:06 > :628: error: unclosed character literal
14:48:06 val chars = List('a', 'b', '???', Char.MaxValue, '\u')
That line shows up differently in my editor
val chars = List('a', 'b', '乕', Char.MaxValue, '\u')
Maybe Jenkins is using a different character encoding?
--
To view, visit http://gerrit.cloudera.org:8080/11030
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c
Gerrit-Change-Number: 11030
Gerrit-PatchSet: 6
Gerrit-Owner: Tony Foerster
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tony Foerster
Gerrit-Comment-Date: Wed, 25 Jul 2018 01:23:52 +
Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 25 Jul 2018 01:19:32 + Gerrit-HasComments: No
[kudu-CR] hms-tool: refactor check tool and combine upgrade and fix
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11018 )
Change subject: hms-tool: refactor check tool and combine upgrade and fix
..
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/11018/5/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:
http://gerrit.cloudera.org:8080/#/c/11018/5/src/kudu/hms/hms_catalog.cc@166
PS5, Line 166: Status HmsCatalog::DropLegacyTable(const string& name) {
Is there a way to reduce the duplicated code between DropLegacyTable and
DropTable?
http://gerrit.cloudera.org:8080/#/c/11018/7/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11018/7/src/kudu/tools/kudu-tool-test.cc@2210
PS7, Line 2210: }
I think it still worth to enable HMS integration after this and see if check
and fix tool works as expected.
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@169
PS6, Line 169: vector*
Why not use const ref?
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@238
PS6, Line 238: duplicate_hms_tables
I don't see how duplicate HMS tables are handled in the fix tool. If we expect
this to be manually fixed, we should at least log "manual fix is required" in
the fix tool?
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@410
PS6, Line 410: drop_orphan_hms_tables
Another option to fix the orphan HMS tables (other than dropping the table) is
to add the corresponding Kudu tables. Do you think it is worth to add such
option in cases that the tables in Kudu were somehow accidentally dropped?
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@451
PS6, Line 451: rename the Kudu table to a Hive-compatible
Why not ask user to input the desired name here?
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@474
PS6, Line 474: table_name
Should we use normalized table name here?
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@532
PS6, Line 532: UpgradeLegacyImpalaTable
I think we cannot simply return early if upgrade HMS tables failed. Since the
legacy table name in Kudu has been already renamed. For any kinds of errors we
may need to roll back the change in previous step.
http://gerrit.cloudera.org:8080/#/c/11018/6/src/kudu/tools/tool_action_hms.cc@548
PS6, Line 548: This will
: // also overwrite any other stale metadata in the HMS
since it's not a
: // Kudu-catalog-only alter.
I don't see any metadata change in the HMS from L551 to L568.
--
To view, visit http://gerrit.cloudera.org:8080/11018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f
Gerrit-Change-Number: 11018
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 25 Jul 2018 01:15:46 +
Gerrit-HasComments: Yes
[kudu-CR] hms-tool: refactor check tool and combine upgrade and fix
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11018 to look at the new patch set (#7). Change subject: hms-tool: refactor check tool and combine upgrade and fix .. hms-tool: refactor check tool and combine upgrade and fix This commit combines the 'hms upgrade' and 'hms fix' tools. The tools previously had overlapping responsibilities for checking some types of inconsistencies. The 'fix' tool now has flags which can control whether it attempts certain types of fixes: --drop_orphan_hms_tables=false --create_missing_hms_tables=true --fix_inconsistent_tables=true --upgrade_hms_tables=true `drop_orphan_hms_tables` defaults to false since it deletes Hive tables, and the tool can not rule-out that they are being used. Additionally, a --dryrun flag is provided in order to print steps that would be taken without actually making modifications. The checking logic has been expanded to account for more potential inconsistencies, and where possible the fix tool now can automatically repair these inconsistencies. Finally, the input prompt and default database functionality has been removed in order to simplify the tool. Instead, the check tool will issue hints including instructions for how to rename tables without a database or with a Hive-incompatible name using the 'kudu table rename_table' tool. We can always add this functionality back in the future if we determine it helps out users. Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f --- M src/kudu/gutil/strings/escaping.h M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 6 files changed, 890 insertions(+), 855 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/11018/7 -- To view, visit http://gerrit.cloudera.org:8080/11018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f Gerrit-Change-Number: 11018 Gerrit-PatchSet: 7 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] hms tools: do not require HMS configuration flags
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11036 to look at the new patch set (#4). Change subject: hms tools: do not require HMS configuration flags .. hms tools: do not require HMS configuration flags The HMS tools will now lookup required HMS configuration (hive_metastore_uris and hive_metastore_sasl_enabled) from the master when it is not provided via command line flags. Looking up the configs on the master is more ergonomic and less error-prone. Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 4 files changed, 99 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/4 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add a way to pin clean time advancement
David Ribeiro Alves has removed a vote on this change. Change subject: Add a way to pin clean time advancement .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/9641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: If863d28016aee98672d94ec5dc8a8630e1cbf5c8 Gerrit-Change-Number: 9641 Gerrit-PatchSet: 7 Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a way to pin clean time advancement
David Ribeiro Alves has posted comments on this change. (
http://gerrit.cloudera.org:8080/9641 )
Change subject: Add a way to pin clean time advancement
..
Patch Set 7: Verified+1
(24 comments)
Oops had some unposted old comments.
The failure is due to an unrelated flake.
http://gerrit.cloudera.org:8080/#/c/9641/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9641/3//COMMIT_MSG@12
PS3, Line 12: mvcc
> nit: MVCC
Done
http://gerrit.cloudera.org:8080/#/c/9641/3//COMMIT_MSG@15
PS3, Line 15: add
> nit: adds
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@226
PS3, Line 226: transaction
> nit: transactions?
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@248
PS3, Line 248: the
> nit: drop
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@255
PS3, Line 255: // Pin clean time advancement to an instant before the
in-flight
> nit: add a stop in the end of the sentence.
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@268
PS3, Line 268: advance
> advances
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc-test.cc@270
PS3, Line 270: safe_time = Timestamp(55);
: mgr.AdjustSafeTime(Timestamp(55));
: ASSERT_EQ(safe_time, mgr.GetCleanTimestamp());
> I'm not sure this corresponds to the comment above: the transaction timesta
good catch! thanks. fixed
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@288
PS3, Line 288: struct ScopedCleanTimeAdvancePin {
> this should probably be marked DISALLOW_COPY_AND_ASSIGN since an accidental
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@289
PS3, Line 289: _
> nit: extra suffix. I think it should be possible to have mvcc(mvcc) in the
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@290
PS3, Line 290:
> nit: spacing/alignment
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@295
PS3, Line 295: mvcc->UnPinCleanTimeAdvance();
> is this idempotent? It surprises me that we don't need to pass in pinned_ti
I avoided making sure that this unpins only once because it makes usage
slightly better in the case of bail out. happy to enforce the check if you feel
strongly.
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@303
PS3, Line 303: Timestamp pinned_timestamp;
> nit: const?
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@370
PS3, Line 370: PinCleanTimeAdvanceTo
> nit: how about PinCleanTimeAdvancement() and UnpinCleanTimeAdvancement().
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@370
PS3, Line 370: void PinCleanTimeAdvanceTo(Timestamp timestamp);
> is there a limitation that there can be only a single pin at a time? What h
yes it crashed added a comment to make that clear
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.h@375
PS3, Line 375: // If clean time is not pinned this has no effect.
> should we be able to FATAL in that case? Seems a little odd that we could h
I didn't make this crash in this case just because the raii use is slightly
cleaner. happy to change it if you feel strongly
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@216
PS3, Line 216: std::lock_guard l(lock_);
> nit: does it make sense to add
it does! thanks
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@217
PS3, Line 217: // There should only be one pin at a time.
> yea, I commented on the header before looking at the impl, and that wasn't
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@217
PS3, Line 217: // There should only be one pin at a time.
> Hrmm.. Is this because the prepare thread is single-threaded, and so we are
yes, but I din't want to talk about prepare stuff here. added a comment to the
header to make it clean that this can only be called once.
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@217
PS3, Line 217: // There should only be one pin at a time.
> Yep, maybe it's worth extend the reasoning a bit in the comment.
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@218
PS3, Line 218: clean_time_pin_, Timestamp::kInvalidTimestamp
> nit: maybe, swap the arguments for better readability? I mean, usually in
Done
http://gerrit.cloudera.org:8080/#/c/9641/3/src/kudu/tablet/mvcc.cc@218
PS3, Line 218: CHECK_EQ(clean_time_pin_, Timestamp::kInvalidTimestamp);
> Are there any other requirements on the clean-time pin? eg it should be ill
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11037 ) Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. [Java] Retry tests that don’t inherit from BaseKuduTest The Scala tests and a couple special case tests don’t inherit from BaseKuduTest but may still be flaky. To fix this I moved the retry configuration logic into the RetryRule and added the rule to the a few tests directly: - TestSecurity - TestKuduMetastorePlugin Additionally I updated the Scala TestContext, now called KuduTestSuite, to use the JUnitSuite and include the RetryRule so that all of the Scala tests would be retried. This required a few syntax changes but has the added benefit of not needing `@RunWith(classOf[JUnitRunner])` at the top of each test class. It also follows similar syntax to the rest of our tests. Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Reviewed-on: http://gerrit.cloudera.org:8080/11037 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java M java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java M java/kudu-hive/build.gradle M java/kudu-hive/pom.xml M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala R java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala 13 files changed, 196 insertions(+), 125 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add alter external catalogs flag to table rename tool
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11017 ) Change subject: Add alter_external_catalogs flag to table rename tool .. Add alter_external_catalogs flag to table rename tool It's going to be necessary to be able to alter the Kudu catalog independently of the HMS in order to repair and upgrade tables when the HMS integration is turned on. A follow-up commit will include a test that exercises this flag when the HMS integration is enabled. Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Reviewed-on: http://gerrit.cloudera.org:8080/11017 Reviewed-by: Hao Hao Tested-by: Dan Burkert --- M src/kudu/client/client.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 6 files changed, 27 insertions(+), 17 deletions(-) Approvals: Hao Hao: Looks good to me, approved Dan Burkert: Verified -- To view, visit http://gerrit.cloudera.org:8080/11017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Gerrit-Change-Number: 11017 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add alter external catalogs flag to table rename tool
Dan Burkert has removed a vote on this change. Change subject: Add alter_external_catalogs flag to table rename tool .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Gerrit-Change-Number: 11017 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add alter external catalogs flag to table rename tool
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11017 ) Change subject: Add alter_external_catalogs flag to table rename tool .. Patch Set 3: Verified+1 unrelated flake -- To view, visit http://gerrit.cloudera.org:8080/11017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Gerrit-Change-Number: 11017 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 23:54:15 + Gerrit-HasComments: No
[kudu-CR] hms tools: do not require HMS configuration flags
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 3: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@a599 PS1, Line 599: > Yes, by removing them as optional parameters they will not be shown in the Thank you for the explanation. Yep, the reasoning makes sense to me. -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 23:03:44 + Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113 PS1, Line 113: FLAGS_hive_metastore_uris.empty() > If the flag is set by the command line but is different from the one specif I think i'd rather keep this simple and skip looking up the config from the master in that case. It should be an exceptional circumstance that the flag gets set manually, and I don't expect it to happen accidentally because its no longer listed as an optional flag in the help output. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master_addrs[0] > Sounds reasonable to me. Should we add a TODO for it? Done -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 23:01:05 + Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11036 to look at the new patch set (#3). Change subject: hms tools: do not require HMS configuration flags .. hms tools: do not require HMS configuration flags The HMS tools will now lookup required HMS configuration (hive_metastore_uris and hive_metastore_sasl_enabled) from the master when it is not provided via command line flags. Looking up the configs on the master is more ergonomic and less error-prone. Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 4 files changed, 97 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/3 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11037 ) Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 22:58:01 + Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
..
Patch Set 7:
(1 comment)
> I'm curious how this will plumb through all the way to the client. Have you
> started working on any such patches for TabletService, etc?
Not yet, no.
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc@1302
PS7, Line 1302: if (ContainsKey(Schema::kVirtualColumnNames, col_name)) {
> Instead of the set, maybe we should just reserve all identifiers of the for
Yeah, Grant had the same feedback. I'm open to making that change; just wanted
to make sure this is headed in the right direction first (see my back and forth
with Dan).
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:57:41 +
Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with is deleted virtual column
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10990 ) Change subject: memrowset: support iteration with is_deleted virtual column .. Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset-test.cc@737 PS5, Line 737: } same comment about testing the case with reinserts, just for good measure http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.h@594 PS5, Line 594: size_t projection_vc_is_deleted_idx_; shouldn't this be int? I think kColumnNotFound is -1 http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.cc@404 PS5, Line 404: projection if projection is a schema, could we just use find_column here? http://gerrit.cloudera.org:8080/#/c/10990/5/src/kudu/tablet/memrowset.cc@562 PS5, Line 562: *reinterpret_cast(dst_row.mutable_cell_ptr(projection_vc_is_deleted_idx_)) = true; Can you use UnalignedStore here instead? It compiles to the same thing but we've been trying to minimize reinterpret_cast usage after KUDU-2378 -- To view, visit http://gerrit.cloudera.org:8080/10990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 Gerrit-Change-Number: 10990 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 22:57:29 + Gerrit-HasComments: Yes
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11037 to look at the new patch set (#3). Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. [Java] Retry tests that don’t inherit from BaseKuduTest The Scala tests and a couple special case tests don’t inherit from BaseKuduTest but may still be flaky. To fix this I moved the retry configuration logic into the RetryRule and added the rule to the a few tests directly: - TestSecurity - TestKuduMetastorePlugin Additionally I updated the Scala TestContext, now called KuduTestSuite, to use the JUnitSuite and include the RetryRule so that all of the Scala tests would be retried. This required a few syntax changes but has the added benefit of not needing `@RunWith(classOf[JUnitRunner])` at the top of each test class. It also follows similar syntax to the rest of our tests. Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java M java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java M java/kudu-hive/build.gradle M java/kudu-hive/pom.xml M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala R java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala 13 files changed, 196 insertions(+), 125 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/11037/3 -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] schema: add is deleted virtual column
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/10968 )
Change subject: schema: add is_deleted virtual column
..
Patch Set 7:
(1 comment)
I'm curious how this will plumb through all the way to the client. Have you
started working on any such patches for TabletService, etc?
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/10968/7/src/kudu/master/catalog_manager.cc@1302
PS7, Line 1302: if (ContainsKey(Schema::kVirtualColumnNames, col_name)) {
Instead of the set, maybe we should just reserve all identifiers of the form
$$*$$? Or if we are afraid someone already named such a column, $$kudu.*$$?
--
To view, visit http://gerrit.cloudera.org:8080/10968
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e
Gerrit-Change-Number: 10968
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:50:07 +
Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with include deleted rows
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10929 ) Change subject: memrowset: support iteration with include_deleted_rows .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset-test.cc File src/kudu/tablet/memrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset-test.cc@585 PS5, Line 585: INSTANTIATE_TEST_CASE_P(MrsConfigurations, ParameterizedTestMemRowSet, : ::testing::Bool()); mind naming this parameterization something more useful to indicate what the value of the bool means? and perhaps add a comment? http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset-test.cc@665 PS5, Line 665: ASSERT_OK(InsertRow(mrs.get(), "row 1", 0)); I think its' worth adding a third row which has been inserted, deleted, and reinserted and maybe some other rows which include some updates in there, too? http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/10929/5/src/kudu/tablet/memrowset.h@557 PS5, Line 557: // The 'was_deleted' parameter will be set to true if the last mutation walked ah, this is starting to look a bit closer to my proposal to add an enum out-param instead of the two bools. Are all four combinations of these two bools possible? If so, maybe two bools is better than one enum. -- To view, visit http://gerrit.cloudera.org:8080/10929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie018043518b437ecc719cf9f87b2c5eea560c9a1 Gerrit-Change-Number: 10929 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 22:42:45 + Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2211 PS1, Line 2211: Kerberos is enabled > I opted to have this one be kerberized and TestCheckAndManualFixHmsMetadata Makes sense. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113 PS1, Line 113: FLAGS_hive_metastore_uris.empty() > hive_metastore_uris won't be empty if it's set on the command line via a fl If the flag is set by the command line but is different from the one specified in master, should log a warning so that users knows what he is doing in this case? http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master_addrs[0] > Eventually I think we should add a check that all masters have the same con Sounds reasonable to me. Should we add a TODO for it? -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 22:39:41 + Gerrit-HasComments: Yes
[kudu-CR] memrowset: support iteration with snap to exclude
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/10926 )
Change subject: memrowset: support iteration with snap_to_exclude
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@514
PS7, Line 514: bool unset_in_sel_vector
this name is making me a little confused, because it can change again after
it's set here. For the purposes of code clarity, do you think this could be
something like:
// Assume the row should be included unless we determine otherwise below.
bool unset_in_sel_vector = false;
bool insertion_excluded_by_snaps = ...;
if (insertion_excluded_by_snaps || opts_.snap_to_include.IsCommitted(...)) {
// maybe use an enum to take the out-param of ApplyMutations, like
NONE_APPLIED, APPLIED_AND_DELETED, APPLIED_AND_PRESENT
ApplyMutations(...);
unset_in_sel_vector = mut_status == APPLIED_AND_DELETED || (mut_status ==
NONE_APPLIED && insertion_excluded_by_snap);
} else {
unset_in_sel_vector = true;
}
I think the code is equivalent, just trying to find something that's a little
easier to follow here without trying to track this tri-state of
unset_in_sel_vector through multiple functions where it might get set or left
alone in various places
http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@517
PS7, Line 517: if (has_upper_bound() && out_of_bounds(k)) {
: state_ = kFinished;
: break;
I wonder if this actually belongs outside the if condition? Even if we exclude
some row due to MVCC, if we see that we've reached a key above our scan bounds,
we can stop at the point instead of continuing until we find a non-excluded row
(which would necesarily be out of bounds as well)
(it seems like it should have been like this before as well, not new in your
change, but maybe would make this look a little less complex)
--
To view, visit http://gerrit.cloudera.org:8080/10926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce
Gerrit-Change-Number: 10926
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 24 Jul 2018 22:36:09 +
Gerrit-HasComments: Yes
[kudu-CR] Add alter external catalogs flag to table rename tool
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11017 ) Change subject: Add alter_external_catalogs flag to table rename tool .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Gerrit-Change-Number: 11017 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 22:25:45 + Gerrit-HasComments: No
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11037 ) Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@95 PS1, Line 95: public RetryRule retryRule = new RetryRule(); > Each test adjust the MiniKuduCluster options a bit. It's likely we could wo Okay, different patch sounds fine. http://gerrit.cloudera.org:8080/#/c/11037/2/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java File java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java: http://gerrit.cloudera.org:8080/#/c/11037/2/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java@31 PS2, Line 31: ,0 Nit: space between comma and 0 -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 22:15:57 + Gerrit-HasComments: Yes
[kudu-CR] hms tools: do not require HMS configuration flags
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2193 PS1, Line 2193: master_addr > nit: master_addrs. And also in all other places. Pretty much all the test cases in this file are using 'master_addr', I think because the tests aren't configured to with multi-master. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2211 PS1, Line 2211: Kerberos is enabled > Do we need to parameterized this with Kerberos disabled/enabled? I opted to have this one be kerberized and TestCheckAndManualFixHmsMetadata not be kerberized. I think that will give us enough coverage without having to execute the test-cases twice. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h@115 PS1, Line 115: // explicitly set. > Can you add a bit explanation/comment on what is the parameter for? And how Done http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@a599 PS1, Line 599: > The comment for the changelist says something like '... when it is not prov Yes, by removing them as optional parameters they will not be shown in the output of 'kudu hms check --help', however they are still settable. In effect, they are 'hidden'. I'd prefer them to not be shown, because the vast majority of the time they should be looked up on the master. The fallback is in place in case a different HMS address must be used, but I can't imagine at this point why you'd want to do that. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113 PS1, Line 113: FLAGS_hive_metastore_uris.empty() > When the URIs and SASL flags will not be empty? Do we need to validate if t hive_metastore_uris won't be empty if it's set on the command line via a flag. In that case it's used instead of the master config because the user is setting it specifically. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master_addrs[0] > Is it possible that different masters have different configs? Eventually I think we should add a check that all masters have the same configuration value, but I think that would better be handled as it's own check as a part of ksck or 'hms check'. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master::Master::kDefaultPort > What happens if the port being used is different from the default? The provided port is the fallback in case the master address doesn't include a port. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@120 PS1, Line 120: hive_metastore_uris > nit: does it make sense to introduce constant literals for 'hive_metastore These are derivative from the associated flag definitions. Ideally there would be a way to get the flag name from the flag symbol (FLAGS_hive_metastore_uris -> 'hive_metastore_uris'), so that if the flag name ever changes this code would stop compiling. I can't figure out a way to do that, though, and I don't think there's a lot of value in defining a constant otherwise, since it isn't the definitive version. -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 22:15:19 + Gerrit-HasComments: Yes
[kudu-CR] Add alter external catalogs flag to table rename tool
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11017 ) Change subject: Add alter_external_catalogs flag to table rename tool .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11017/2/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11017/2/src/kudu/client/client.h@61 PS2, Line 61: class KuduTableAlterer; > nit: this can be moved to L71. Done -- To view, visit http://gerrit.cloudera.org:8080/11017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Gerrit-Change-Number: 11017 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 22:15:02 + Gerrit-HasComments: Yes
[kudu-CR] Add alter external catalogs flag to table rename tool
Hello Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11017 to look at the new patch set (#3). Change subject: Add alter_external_catalogs flag to table rename tool .. Add alter_external_catalogs flag to table rename tool It's going to be necessary to be able to alter the Kudu catalog independently of the HMS in order to repair and upgrade tables when the HMS integration is turned on. A follow-up commit will include a test that exercises this flag when the HMS integration is enabled. Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 --- M src/kudu/client/client.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 6 files changed, 27 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/11017/3 -- To view, visit http://gerrit.cloudera.org:8080/11017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Gerrit-Change-Number: 11017 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms precheck tool
Hello Adar Dembo, Hao Hao, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11040 to review the following change. Change subject: hms precheck tool .. hms precheck tool This commit adds a 'kudu hms precheck' tool that is meant to be used before enabling the HMS integration on an existing cluster. The tool checks for issues that cause the master to fail to startup after enabling the HMS flags. Change-Id: Ia7b23cb802b6434eba7e38443d3361ef70e6543e --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 2 files changed, 133 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/11040/1 -- To view, visit http://gerrit.cloudera.org:8080/11040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia7b23cb802b6434eba7e38443d3361ef70e6543e Gerrit-Change-Number: 11040 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao
[kudu-CR] hms tools: do not require HMS configuration flags
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11036 to look at the new patch set (#2). Change subject: hms tools: do not require HMS configuration flags .. hms tools: do not require HMS configuration flags The HMS tools will now lookup required HMS configuration (hive_metastore_uris and hive_metastore_sasl_enabled) from the master when it is not provided via command line flags. Looking up the configs on the master is more ergonomic and less error-prone. Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 4 files changed, 95 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/2 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [rpc tests] Reduce flakyness by setting a longer keepalive in TSAN
David Ribeiro Alves has abandoned this change. ( http://gerrit.cloudera.org:8080/6289 ) Change subject: [rpc tests] Reduce flakyness by setting a longer keepalive in TSAN .. Abandoned I no longer have a patch around I can test this on, and it hasn't come up since so abandoning -- To view, visit http://gerrit.cloudera.org:8080/6289 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ic93df66e312239fa3de22e5d833c27982e0683d9 Gerrit-Change-Number: 6289 Gerrit-PatchSet: 1 Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Scala code formatting with Scalafmt
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 ) Change subject: Scala code formatting with Scalafmt .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/11030/6/java/.gitignore File java/.gitignore: http://gerrit.cloudera.org:8080/#/c/11030/6/java/.gitignore@39 PS6, Line 39: **/out let me know if you want this as part of a different commit. http://gerrit.cloudera.org:8080/#/c/11030/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala: http://gerrit.cloudera.org:8080/#/c/11030/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala@25 PS3, Line 25: case Type.BOOL => BooleanType > You may need to undo the changes and then re-run the formatting. My underst Check out patch set 6, it's fixed there. The formatter will un-align, this patch set was not reading .scalafmt.conf and was instead using the default. -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 6 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Tue, 24 Jul 2018 22:05:43 + Gerrit-HasComments: Yes
[kudu-CR] [NOT FOR REVIEW WIP] Design doc for repeatable reads
David Ribeiro Alves has abandoned this change. ( http://gerrit.cloudera.org:8080/5229 ) Change subject: [NOT FOR REVIEW WIP] Design doc for repeatable reads .. Abandoned no longer relevant since was largely fixed in other ways by hao hao and myself -- To view, visit http://gerrit.cloudera.org:8080/5229 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ia910063b82dd867259438895e930cc6baed045ae Gerrit-Change-Number: 5229 Gerrit-PatchSet: 10 Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [flaky tests] Increase the result ttl for ExactlyOnceRpcTest.TestExactlyOnceSemanticsGarbageCollectionStressTest
David Ribeiro Alves has abandoned this change. ( http://gerrit.cloudera.org:8080/4644 ) Change subject: [flaky tests] Increase the result ttl for ExactlyOnceRpcTest.TestExactlyOnceSemanticsGarbageCollectionStressTest .. Abandoned abandoning this, not the root cause of the problems -- To view, visit http://gerrit.cloudera.org:8080/4644 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I1257ddaf02dfca05829edc904b2f4ff406b933ca Gerrit-Change-Number: 4644 Gerrit-PatchSet: 1 Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Simplify OpId/Timestamp assignment and make it atomic
Hello Tidy Bot, Mike Percy, Alexey Serbin, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7221 to look at the new patch set (#28). Change subject: Simplify OpId/Timestamp assignment and make it atomic .. Simplify OpId/Timestamp assignment and make it atomic There are a couple of constraints that are tricky to enforce and were requiring us to have convoluted interactions on OpId/Timestamp assignment and registration between the TimeManager, MvccManager and ConsensusQueue. These convoluted interactions are currently preventing a clean and simple leader leases implementation, particularly the single timestamp outstanding in TimeManager. The "tricky" constraints are: i) Before clean time is advanced in mvcc, all transactions with a timestamp lower than the current time must be registered with mvcc. This allows to wait for a certain timestamp t to be "safe" with TimeManager and then wait for all transactions with a timestamp t1 < t to commit, thus making sure that a scan at t is repeatable. ii) The leader can't advance follower safe timestamp to t before all the operations with a timestamp lower than it have also been sent. This is hard since timestamps are assigned outside of consensus. In general it's problematic to generate OpIds non-atomically with timestamps, since these are both supposed to be monotonically increasing in unison. To address this we had some complex state and interactions between the components. For instance this was requiring us to have TimeManager stop safe time advancement until a transaction being prepared was submitted to the queue, after assigning a timestamp in the TransactionDriver. This was problematic because with would only allow at most one assigned timestamp in-flight, meaning we were not able to move safe time at will. This patch completely addresses ii), Timestamps and OpIds are now assigned by PeerMessageQueue, atomically. This makes it trivial to make sure that the safe time sent to replicas is inline with the sent messages. This also allows some simplifications and refactorings of the PeerMessageQueue and TimeManager. As for i) the way we enforce this constraint changed. Instead of registering a transaction before submitting it to consensus (which we now can't since consensus submission is also where we assign the timestamp) we register another, dummy transaction on the MvccManager before submission and abort it afterwards. This allows to "pin" safe time advancement down to a timestamp that is mandatorily smaller than then one that will be assigned thus enforcing the constraint with a simpler implementation at the cost of an additional (short-lived) transaction in mvcc. Note in this regard that there will be at most one aditional transaction in mvcc. One side effect of this change is that, for new config changes, we won't know the OpId until after the queue submission, which needs to happen after actually setting the pending config. This is largely inconsequential since, by design, we can have at most one pending config, but did require some refactoring. Change-Id: I2bedcc5c3708d06f48131f34ea14c835867b4800 --- M java/kudu-client/src/test/java/org/apache/kudu/client/TestHybridTime.java M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_peers-test.cc M src/kudu/consensus/consensus_queue-test.cc M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/consensus_queue.h M src/kudu/consensus/log_cache-test.cc M src/kudu/consensus/pending_rounds.cc M src/kudu/consensus/quorum_util.cc M src/kudu/consensus/quorum_util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/time_manager-test.cc M src/kudu/consensus/time_manager.cc M src/kudu/consensus/time_manager.h M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/test_workload.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/ts_tablet_manager.cc 24 files changed, 358 insertions(+), 284 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/7221/28 -- To view, visit http://gerrit.cloudera.org:8080/7221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2bedcc5c3708d06f48131f34ea14c835867b4800 Gerrit-Change-Number: 7221 Gerrit-PatchSet: 28 Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a way to pin clean time advancement
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Hao Hao, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9641 to look at the new patch set (#7). Change subject: Add a way to pin clean time advancement .. Add a way to pin clean time advancement In order to make OpId/Timestamp assignment to transactions atomic, during the prepare phase, we need to let consensus assign the timestamp, before starting a transaction in mvcc. However we also need to make sure that clean time is not advanced past the possible timestamps assigned. This patch add a pin to clean time advancement that acts much like safe time, in the sense that it doesn't allow clean time to be advanced past it. This also adds a way to create and delete a pin within a scope in a raii way. Change-Id: If863d28016aee98672d94ec5dc8a8630e1cbf5c8 --- M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h 3 files changed, 75 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9641/7 -- To view, visit http://gerrit.cloudera.org:8080/9641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If863d28016aee98672d94ec5dc8a8630e1cbf5c8 Gerrit-Change-Number: 9641 Gerrit-PatchSet: 7 Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] hms tools: do not require HMS configuration flags
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2193 PS1, Line 2193: master_addr nit: master_addrs. And also in all other places. http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/kudu-tool-test.cc@2211 PS1, Line 2211: Kerberos is enabled Do we need to parameterized this with Kerberos disabled/enabled? http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_common.h@115 PS1, Line 115: bool all_flags, Can you add a bit explanation/comment on what is the parameter for? And how it would work with 'flag_tags'? http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@113 PS1, Line 113: FLAGS_hive_metastore_uris.empty() When the URIs and SASL flags will not be empty? Do we need to validate if the flags' value match with the ones in master in this case? http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master_addrs[0] Is it possible that different masters have different configs? http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@116 PS1, Line 116: master::Master::kDefaultPort What happens if the port being used is different from the default? http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@120 PS1, Line 120: hive_metastore_uris > nit: does it make sense to introduce constant literals for 'hive_metastore +1 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 21:52:55 + Gerrit-HasComments: Yes
[kudu-CR] [gradle] Add rerunTests property
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11035 ) Change subject: [gradle] Add rerunTests property .. [gradle] Add rerunTests property Gradle keeps track of changes and will skip tasks that are up-to-date when possible. This is great for incremental build speed, but can work against you if you want to rerun tests multiple times. Gradle does have a —rerun-tasks flag that will make sure the tests are run. However, it will also rerun all the tasks in the build tree. This patch adds a rerunTests property that will tell gradle the tests need to be rerun when passed. Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c Reviewed-on: http://gerrit.cloudera.org:8080/11035 Reviewed-by: Adar Dembo Tested-by: Grant Henke --- M java/gradle/tests.gradle 1 file changed, 5 insertions(+), 0 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Grant Henke: Verified -- To view, visit http://gerrit.cloudera.org:8080/11035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c Gerrit-Change-Number: 11035 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] hms tools: do not require HMS configuration flags
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11036 ) Change subject: hms tools: do not require HMS configuration flags .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@a599 PS1, Line 599: The comment for the changelist says something like '... when it is not provided via command line flags ...'. Is that intended that those flags became hidden once they are removed from the list of optional flags for the HMS-related CLI actions? http://gerrit.cloudera.org:8080/#/c/11036/1/src/kudu/tools/tool_action_hms.cc@120 PS1, Line 120: hive_metastore_uris nit: does it make sense to introduce constant literals for 'hive_metastore_uris' and 'hive_metastore_sasl_enabled' flag names? -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 21:36:20 + Gerrit-HasComments: Yes
[kudu-CR] [gradle] Add rerunTests property
Grant Henke has removed a vote on this change. Change subject: [gradle] Add rerunTests property .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c Gerrit-Change-Number: 11035 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [gradle] Add rerunTests property
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11035 ) Change subject: [gradle] Add rerunTests property .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c Gerrit-Change-Number: 11035 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 21:36:54 + Gerrit-HasComments: No
[kudu-CR] Add alter external catalogs flag to table rename tool
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11017 ) Change subject: Add alter_external_catalogs flag to table rename tool .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11017/2/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11017/2/src/kudu/client/client.h@61 PS2, Line 61: class KuduClient; nit: this can be moved to L71. -- To view, visit http://gerrit.cloudera.org:8080/11017 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f5d97f234283b7740df727e085523c7f0b09735 Gerrit-Change-Number: 11017 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 21:18:29 + Gerrit-HasComments: Yes
[kudu-CR] Scala code formatting with Scalafmt
Hello Mike Percy, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11030 to look at the new patch set (#5). Change subject: Scala code formatting with Scalafmt .. Scala code formatting with Scalafmt Scalafmt is added using the Gradle Scalafmt plugin: https://github.com/alenkacz/gradle-scalafmt. Scalafmt https://scalameta.org/scalafmt/#Scalafmt is a code formatting tool that runs from Gradle or as an IDE plugin. The .scalafmt.conf is the main configuration file for scalafmt. The plugin is configured to run on compile or can invoked with 'gradle scalafmt' or 'gradle testScalafmt' to format test code Change-Id: Iac96383d88394084e19712177d05f9fc63de766c --- M java/.gitignore A java/.scalafmt.conf M java/buildSrc/build.gradle M java/gradle/quality.gradle M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala 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/KuduRDD.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduWriteOptions.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.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 M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala 24 files changed, 1,247 insertions(+), 720 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/11030/5 -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 5 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster
[kudu-CR] Scala code formatting with Scalafmt
Hello Mike Percy, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11030 to look at the new patch set (#4). Change subject: Scala code formatting with Scalafmt .. Scala code formatting with Scalafmt Scalafmt is added using the Gradle Scalafmt plugin: https://github.com/alenkacz/gradle-scalafmt. Scalafmt https://scalameta.org/scalafmt/#Scalafmt is a code formatting tool that runs from Gradle or as an IDE plugin. The .scalafmt.conf is the main configuration file for scalafmt. The plugin is configured to run on compile or can invoked with 'gradle scalafmt' or 'gradle testScalafmt' to format test code Change-Id: Iac96383d88394084e19712177d05f9fc63de766c --- M java/.gitignore A java/.scalafmt.conf M java/buildSrc/build.gradle M java/gradle/quality.gradle M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala 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/KuduRDD.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduWriteOptions.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.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 M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala 24 files changed, 1,248 insertions(+), 720 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/11030/4 -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 4 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster
[kudu-CR] exactly once writes-itest for faulty disks
David Ribeiro Alves has posted comments on this change. (
http://gerrit.cloudera.org:8080/8388 )
Change subject: exactly_once_writes-itest for faulty disks
..
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc
File src/kudu/integration-tests/exactly_once_writes-itest.cc:
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@77
PS8, Line 77: using cluster::ExternalTabletServer;
nit: move before the two above
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@83
PS8, Line 83: int64_t
xs nit: why is this the only int64_t ? doesn't seem like it would be feasible
to have over 2B attempts
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@110
PS8, Line 110: If 'allow_crashes' is set, crashed servers are restarted.
maybe add that if 'allow_crashes' is false the test is caused to fail (that is
what happens, right?)
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@118
PS8, Line 118: matching commit indexes.
not sure what this means...
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@234
PS8, Line 234: {
: std::lock_guard l(lock);
: if (num_complete == num_threads) break;
: }
: if (allow_crashes) {
: RETURN_IF_FATALS(RestartAnyCrashedTabletServers(),
: "Failed to restart crashed tablet servers");
: } else {
: RETURN_IF_FATALS(AssertNoTabletServersCrashed(),
: "Some tablet servers crashed");
: }
: SleepFor(MonoDelta::FromMilliseconds(10));
nit: might be slightly cleaner to use a countdown latch (avoid the extra
synchronization around the shared var and has a timeout function that replaces
the sleep)
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@303
PS8, Line 303: LOG(INFO)
debug? same below
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/util/test_macros.h
File src/kudu/util/test_macros.h:
http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/util/test_macros.h@27
PS8, Line 27: // Sometimes it's useful to return a status, so we're not forced
to write void
: // functions to check for fatals
explain this better, can't figure it out just from reading this without much
more context
--
To view, visit http://gerrit.cloudera.org:8080/8388
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I966b41b71e39827f8f78fcca59a2208f13f71e53
Gerrit-Change-Number: 8388
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 24 Jul 2018 21:14:31 +
Gerrit-HasComments: Yes
[kudu-CR] Allow non-HMS compatible tables to be renamed after turning on HMS integration
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11016 ) Change subject: Allow non-HMS compatible tables to be renamed after turning on HMS integration .. Allow non-HMS compatible tables to be renamed after turning on HMS integration This is a bug fix followup to cee17c03bc30037bf2a, which introduced automatic downcasing of table names during new table creation and table renaming. Change-Id: If554efbda06e33b5fdb40565e700bad8a306c143 Reviewed-on: http://gerrit.cloudera.org:8080/11016 Reviewed-by: Adar Dembo Reviewed-by: Hao Hao Tested-by: Dan Burkert --- M src/kudu/client/client.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.cc 3 files changed, 50 insertions(+), 14 deletions(-) Approvals: Adar Dembo: Looks good to me, but someone else must approve Hao Hao: Looks good to me, approved Dan Burkert: Verified -- To view, visit http://gerrit.cloudera.org:8080/11016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If554efbda06e33b5fdb40565e700bad8a306c143 Gerrit-Change-Number: 11016 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Allow non-HMS compatible tables to be renamed after turning on HMS integration
Dan Burkert has removed a vote on this change. Change subject: Allow non-HMS compatible tables to be renamed after turning on HMS integration .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: If554efbda06e33b5fdb40565e700bad8a306c143 Gerrit-Change-Number: 11016 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Allow non-HMS compatible tables to be renamed after turning on HMS integration
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11016 ) Change subject: Allow non-HMS compatible tables to be renamed after turning on HMS integration .. Patch Set 2: Verified+1 Getting hammered by spark flakes. -- To view, visit http://gerrit.cloudera.org:8080/11016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If554efbda06e33b5fdb40565e700bad8a306c143 Gerrit-Change-Number: 11016 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 21:14:04 + Gerrit-HasComments: No
[kudu-CR] Allow non-HMS compatible tables to be renamed after turning on HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11016 )
Change subject: Allow non-HMS compatible tables to be renamed after turning on
HMS integration
..
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11016/2/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11016/2/src/kudu/integration-tests/master_hms-itest.cc@585
PS2, Line 585: TEST_F(MasterHmsUpgradeTest, TestRenameExistingTables) {
> I believe that's covered by 'TestConflictingNormalizedNames' above, or is t
Sorry, missed that. LGTM.
--
To view, visit http://gerrit.cloudera.org:8080/11016
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If554efbda06e33b5fdb40565e700bad8a306c143
Gerrit-Change-Number: 11016
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 21:09:24 +
Gerrit-HasComments: Yes
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11037 ) Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@95 PS1, Line 95: public RetryRule retryRule = new RetryRule(); > Why can't TestSecurity extend BaseKuduTest? Can its use of MiniKuduCluster Each test adjust the MiniKuduCluster options a bit. It's likely we could work it in to use it. I would prefer to keep it a different patch though. http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java File java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java@30 PS1, Line 30: private static final String RETRY_PROPERTY_NAME = "rerunFailingTestsCount"; : private static final int DEFAULT_RETRY_COUNT = 0; : private static final String retryPropVal = System.getProperty(RETRY_PROPERTY_NAME); : private static final int retryCount = : retryPropVal != null ? Integer.parseInt(retryPropVal) : DEFAULT_RETRY_COUNT; > I think the entire block would be more readable as: definitely. http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala File java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala@32 PS1, Line 32: > Nit: there's an extra space here. Done http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala@29 PS1, Line 29: fail() > Did you mean to add this? No, missed one of the places I was testing retries. -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 21:03:00 + Gerrit-HasComments: Yes
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11037 to look at the new patch set (#2). Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. [Java] Retry tests that don’t inherit from BaseKuduTest The Scala tests and a couple special case tests don’t inherit from BaseKuduTest but may still be flaky. To fix this I moved the retry configuration logic into the RetryRule and added the rule to the a few tests directly: - TestSecurity - TestKuduMetastorePlugin Additionally I updated the Scala TestContext, now called KuduTestSuite, to use the JUnitSuite and include the RetryRule so that all of the Scala tests would be retried. This required a few syntax changes but has the added benefit of not needing `@RunWith(classOf[JUnitRunner])` at the top of each test class. It also follows similar syntax to the rest of our tests. Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java M java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java M java/kudu-hive/build.gradle M java/kudu-hive/pom.xml M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala R java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala 13 files changed, 196 insertions(+), 125 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/11037/2 -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Allow non-HMS compatible tables to be renamed after turning on HMS integration
Dan Burkert has posted comments on this change. (
http://gerrit.cloudera.org:8080/11016 )
Change subject: Allow non-HMS compatible tables to be renamed after turning on
HMS integration
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11016/2/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11016/2/src/kudu/integration-tests/master_hms-itest.cc@585
PS2, Line 585: TEST_F(MasterHmsUpgradeTest, TestRenameExistingTables) {
> Not sure if a test case such that a table with lowercase name is already ex
I believe that's covered by 'TestConflictingNormalizedNames' above, or is there
something it's missing?
--
To view, visit http://gerrit.cloudera.org:8080/11016
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If554efbda06e33b5fdb40565e700bad8a306c143
Gerrit-Change-Number: 11016
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 20:51:44 +
Gerrit-HasComments: Yes
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11037 ) Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@95 PS1, Line 95: public RetryRule retryRule = new RetryRule(); Why can't TestSecurity extend BaseKuduTest? Can its use of MiniKuduCluster be massaged slightly to fit into BaseKuduTest and avoid this? -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 20:45:26 + Gerrit-HasComments: Yes
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11037 )
Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java
File java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java:
http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java@30
PS1, Line 30: private static final String RETRY_PROPERTY_NAME =
"rerunFailingTestsCount";
: private static final int DEFAULT_RETRY_COUNT = 0;
: private static final String retryPropVal =
System.getProperty(RETRY_PROPERTY_NAME);
: private static final int retryCount =
: retryPropVal != null ? Integer.parseInt(retryPropVal) :
DEFAULT_RETRY_COUNT;
I think the entire block would be more readable as:
private static final int RETRY_COUNT =
Integer.getInteger("rerunFailingTestsCount", 0);
http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
File
java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala:
http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala@32
PS1, Line 32:
Nit: there's an extra space here.
http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala
File
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala:
http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala@29
PS1, Line 29: fail()
Did you mean to add this?
--
To view, visit http://gerrit.cloudera.org:8080/11037
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b
Gerrit-Change-Number: 11037
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 20:43:58 +
Gerrit-HasComments: Yes
[kudu-CR] Allow non-HMS compatible tables to be renamed after turning on HMS integration
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11016 )
Change subject: Allow non-HMS compatible tables to be renamed after turning on
HMS integration
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11016/2/src/kudu/integration-tests/master_hms-itest.cc
File src/kudu/integration-tests/master_hms-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11016/2/src/kudu/integration-tests/master_hms-itest.cc@585
PS2, Line 585: TEST_F(MasterHmsUpgradeTest, TestRenameExistingTables) {
Not sure if a test case such that a table with lowercase name is already exist
is needed? e.g. a table named 'default.uppercase' is already exist in this case.
--
To view, visit http://gerrit.cloudera.org:8080/11016
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If554efbda06e33b5fdb40565e700bad8a306c143
Gerrit-Change-Number: 11016
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 24 Jul 2018 20:40:49 +
Gerrit-HasComments: Yes
[kudu-CR] [Java] Retry tests that don’t inherit from BaseKuduTest
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11037 Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest .. [Java] Retry tests that don’t inherit from BaseKuduTest The Scala tests and a couple special case tests don’t inherit from BaseKuduTest but may still be flaky. To fix this I moved the retry configuration logic into the RetryRule and added the rule to the a few tests directly: - TestSecurity - TestKuduMetastorePlugin Additionally I updated the Scala TestContext, now called KuduTestSuite, to use the JUnitSuite and include the RetryRule so that all of the Scala tests would be retried. This required a few syntax changes but has the added benefit of not needing `@RunWith(classOf[JUnitRunner])` at the top of each test class. It also follows similar syntax to the rest of our tests. Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java M java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java M java/kudu-hive/build.gradle M java/kudu-hive/pom.xml M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala R java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduTestSuite.scala 13 files changed, 199 insertions(+), 124 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/11037/1 -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] [tablet] clean-up on replay of WAL entries
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11032 ) Change subject: [tablet] clean-up on replay of WAL entries .. [tablet] clean-up on replay of WAL entries Simplified handling of the ownership of log entries read from disk. This changelist also contains other changes in tablet_bootstrap.cc and corresponding WAL utilities: * reduced number of memory allocations while reading WAL entries * replaced gscoped_ptr with std::unique_ptr * other minor updates around WAL-related utilities and tests Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Reviewed-on: http://gerrit.cloudera.org:8080/11032 Tested-by: Alexey Serbin Reviewed-by: Adar Dembo --- M src/kudu/consensus/log-test-base.h M src/kudu/consensus/log-test.cc M src/kudu/consensus/log_reader.cc M src/kudu/consensus/log_reader.h M src/kudu/consensus/log_util.cc M src/kudu/consensus/log_util.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/log_verifier.cc M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_local_replica.cc 11 files changed, 256 insertions(+), 239 deletions(-) Approvals: Alexey Serbin: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Gerrit-Change-Number: 11032 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tablet] clean-up on replay of WAL entries
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 ) Change subject: [tablet] clean-up on replay of WAL entries .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Gerrit-Change-Number: 11032 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 20:20:31 + Gerrit-HasComments: No
[kudu-CR] Scala code formatting with Scalafmt
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 ) Change subject: Scala code formatting with Scalafmt .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11030/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11030/2//COMMIT_MSG@9 PS2, Line 9: Scalafmt is added using the Gradle Scalafmt plugin: > I saw that Spotless would do scalafmt, but also does a lot more, so I went True it should be easy to swap out. -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 3 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Tue, 24 Jul 2018 20:16:57 + Gerrit-HasComments: Yes
[kudu-CR] Scala code formatting with Scalafmt
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 ) Change subject: Scala code formatting with Scalafmt .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11030/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala: http://gerrit.cloudera.org:8080/#/c/11030/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala@25 PS3, Line 25: case Type.BOOL=> BooleanType > Not sure why it's still aligning case arrows, checking that. You may need to undo the changes and then re-run the formatting. My understanding is it will align the arrows if enabled, but not "unalign" them if disabled. -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 3 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Tue, 24 Jul 2018 20:14:02 + Gerrit-HasComments: Yes
[kudu-CR] [tablet] clean-up on replay of WAL entries
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 ) Change subject: [tablet] clean-up on replay of WAL entries .. Patch Set 2: Verified+1 Unrelated flakes in org.apache.kudu.spark.kudu.DefaultSourceTest.socket read timeout propagation -- To view, visit http://gerrit.cloudera.org:8080/11032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Gerrit-Change-Number: 11032 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 20:02:24 + Gerrit-HasComments: No
[kudu-CR] [tablet] clean-up on replay of WAL entries
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11032 ) Change subject: [tablet] clean-up on replay of WAL entries .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Gerrit-Change-Number: 11032 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tablet] clean-up on replay of WAL entries
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 ) Change subject: [tablet] clean-up on replay of WAL entries .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log-test.cc File src/kudu/consensus/log-test.cc: http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log-test.cc@293 PS1, Line 293: // TODO(unknown): put this in TearDown() with a test on log state? > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h File src/kudu/consensus/log_util.h: http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h@208 PS1, Line 208: // the log entries read up to the corrupted one are returned in the 'entries' > You could use LogEntries here too, if you're willing to define it outside o Done http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc@473 PS1, Line 473: string debug_str = entry_debug_info; > Remove this? Done -- To view, visit http://gerrit.cloudera.org:8080/11032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Gerrit-Change-Number: 11032 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 20:02:04 + Gerrit-HasComments: Yes
[kudu-CR] Scala code formatting with Scalafmt
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 ) Change subject: Scala code formatting with Scalafmt .. Patch Set 3: Code-Review-1 (1 comment) It's still aligning case arrows, please ignore this until I can get that figured out http://gerrit.cloudera.org:8080/#/c/11030/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala: http://gerrit.cloudera.org:8080/#/c/11030/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.scala@25 PS3, Line 25: case Type.BOOL=> BooleanType Not sure why it's still aligning case arrows, checking that. -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 3 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Tue, 24 Jul 2018 19:53:21 + Gerrit-HasComments: Yes
[kudu-CR] Scala code formatting with Scalafmt
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/11030 ) Change subject: Scala code formatting with Scalafmt .. Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11030/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11030/2//COMMIT_MSG@9 PS2, Line 9: Scalafmt is added using the Gradle Scalafmt plugin: > Have you considered using the spotless plugin? I saw that Spotless would do scalafmt, but also does a lot more, so I went with the simpler solution. If we want to add Java formatting as well Spotless might be the way to go, I don't have any experience with it. I'd feel better about doing the commits for scala/java style separately. If spotless was added in the future it should be trivial to use it for scalafmt. http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf File java/.scalafmt.conf: http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf@1 PS2, Line 1: rewrite.rules = [SortImports, sortModifiers, prefercurlyfors, AvoidInfix, expandimportselectors] > I like `expandimportselectors` because it results in less change collisions Done http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf@2 PS2, Line 2: align = none > +1 this setting is pretty aggressive I don't mind the aligned cases (as long as it's done automatically for me), but two on one I'll let it go :) http://gerrit.cloudera.org:8080/#/c/11030/2/java/.scalafmt.conf@9 PS2, Line 9: newlines.alwaysBeforeTopLevelStatements = true > Here is an example of what this setting can do: https://github.com/olafurpg Agreed, the formatting can get a little too vertical. http://gerrit.cloudera.org:8080/#/c/11030/2/java/gradle/quality.gradle File java/gradle/quality.gradle: http://gerrit.cloudera.org:8080/#/c/11030/2/java/gradle/quality.gradle@25 PS2, Line 25: apply plugin: "scalafmt" > nit: newline Done http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-backup/out/test/resources/log4j.properties File java/kudu-backup/out/test/resources/log4j.properties: http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-backup/out/test/resources/log4j.properties@1 PS2, Line 1: > OK I've done this many times now. I added the 'out' dirs to the .gitignore, let me know if you'd rather it be its own commit. http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala: http://gerrit.cloudera.org:8080/#/c/11030/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala@24 PS2, Line 24: * Adds a method, `kudu`, to DataFrameReader that allows you to read Kudu tables using > Why is this using the Scala style comment? Odd, none of them changed to javadoc. Since they all seem to be scaladoc style in the first place I'll switch to that for now. Or maybe it's just broken. -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 3 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tony Foerster Gerrit-Comment-Date: Tue, 24 Jul 2018 19:25:21 + Gerrit-HasComments: Yes
[kudu-CR] Scala code formatting with Scalafmt
Hello Mike Percy, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11030 to look at the new patch set (#3). Change subject: Scala code formatting with Scalafmt .. Scala code formatting with Scalafmt Scalafmt is added using the Gradle Scalafmt plugin: https://github.com/alenkacz/gradle-scalafmt. Scalafmt https://scalameta.org/scalafmt/#Scalafmt is a code formatting tool that runs from Gradle or as an IDE plugin. The .scalafmt.conf is the main configuration file for scalafmt. The plugin is configured to run on compile or can invoked with 'gradle scalafmt' or 'gradle testScalafmt' to format test code Change-Id: Iac96383d88394084e19712177d05f9fc63de766c --- M java/.gitignore A java/.scalafmt.conf M java/buildSrc/build.gradle M java/gradle/quality.gradle M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestoreOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/TableMetadata.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala M java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala 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/KuduRDD.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduWriteOptions.scala M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/SparkUtil.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 M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala 24 files changed, 1,249 insertions(+), 720 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/11030/3 -- To view, visit http://gerrit.cloudera.org:8080/11030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iac96383d88394084e19712177d05f9fc63de766c Gerrit-Change-Number: 11030 Gerrit-PatchSet: 3 Gerrit-Owner: Tony Foerster Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] hms-tool: refactor check tool and combine upgrade and fix
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11018 to look at the new patch set (#5). Change subject: hms-tool: refactor check tool and combine upgrade and fix .. hms-tool: refactor check tool and combine upgrade and fix This commit combines the 'hms upgrade' and 'hms fix' tools. The tools previously had overlapping responsibilities for checking some types of inconsistencies. The 'fix' tool now has flags which can control whether it attempts certain types of fixes: --drop_orphan_hms_tables=false --create_missing_hms_tables=true --fix_inconsistent_tables=true --upgrade_hms_tables=true `drop_orphan_hms_tables` defaults to false since it deletes Hive tables, and the tool can not rule-out that they are being used. Additionally, a --dryrun flag is provided in order to print steps that would be taken without actually making modifications. The checking logic has been expanded to account for more potential inconsistencies, and where possible the fix tool now can automatically repair these inconsistencies. Finally, the input prompt and default database functionality has been removed in order to simplify the tool. Instead, the check tool will issue hints including instructions for how to rename tables without a database or with a Hive-incompatible name using the 'kudu table rename_table' tool. We can always add this functionality back in the future if we determine it helps out users. Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f --- M src/kudu/gutil/strings/escaping.h M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_catalog.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_hms.cc 6 files changed, 890 insertions(+), 855 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/11018/5 -- To view, visit http://gerrit.cloudera.org:8080/11018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f Gerrit-Change-Number: 11018 Gerrit-PatchSet: 5 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Patch Set 2: Verified+1 Overriding Jenkins, more Java flakes. -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 19:05:34 + Gerrit-HasComments: No
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] hms tools: do not require HMS configuration flags
Hello Adar Dembo, Hao Hao, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11036 to review the following change. Change subject: hms tools: do not require HMS configuration flags .. hms tools: do not require HMS configuration flags The HMS tools will now lookup required HMS configuration (hive_metastore_uris and hive_metastore_sasl_enabled) from the master when it is not provided via command line flags. Looking up the configs on the master is more ergonomic and less error-prone. Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 4 files changed, 89 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/11036/1 -- To view, visit http://gerrit.cloudera.org:8080/11036 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibf580384a1612174a75142295d7f604cd6fabfe1 Gerrit-Change-Number: 11036 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao
[kudu-CR] [tablet] clean-up on replay of WAL entries
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11032 to look at the new patch set (#2). Change subject: [tablet] clean-up on replay of WAL entries .. [tablet] clean-up on replay of WAL entries Simplified handling of the ownership of log entries read from disk. This changelist also contains other changes in tablet_bootstrap.cc and corresponding WAL utilities: * reduced number of memory allocations while reading WAL entries * replaced gscoped_ptr with std::unique_ptr * other minor updates around WAL-related utilities and tests Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb --- M src/kudu/consensus/log-test-base.h M src/kudu/consensus/log-test.cc M src/kudu/consensus/log_reader.cc M src/kudu/consensus/log_reader.h M src/kudu/consensus/log_util.cc M src/kudu/consensus/log_util.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/log_verifier.cc M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_local_replica.cc 11 files changed, 256 insertions(+), 239 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/11032/2 -- To view, visit http://gerrit.cloudera.org:8080/11032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Gerrit-Change-Number: 11032 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [gradle] Add rerunTests property
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11035 ) Change subject: [gradle] Add rerunTests property .. Patch Set 1: This is slightly different. Maybe I need a better name for the property. I am working on reproducing the flaky test locally, when I run `gradle test` if I have already run the tests gradle will do nothing and say the task is up-to-date. With this it will just rerun test test task without rerunning everything else (like it would if I used --rerun-tasks). -- To view, visit http://gerrit.cloudera.org:8080/11035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c Gerrit-Change-Number: 11035 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 18:21:31 + Gerrit-HasComments: No
[kudu-CR] [gradle] Add rerunTests property
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11035 ) Change subject: [gradle] Add rerunTests property .. Patch Set 1: (1 comment) So I had a sneaking suspicion that Java test retrying might be broken due to the sheer number of Java flakes that have cropped up in recent pre-commit builds (and not just the backup-related ones). Does this address that? I don't think it does: rerunTests needs to be explicitly set on the command line, and it isn't right now, right? http://gerrit.cloudera.org:8080/#/c/11035/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11035/1//COMMIT_MSG@9 PS1, Line 9: [gradle] Add rerunTests property Duplicate -- To view, visit http://gerrit.cloudera.org:8080/11035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c Gerrit-Change-Number: 11035 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 18:17:30 + Gerrit-HasComments: Yes
[kudu-CR] [tablet] clean-up on replay of WAL entries
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11032 ) Change subject: [tablet] clean-up on replay of WAL entries .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h File src/kudu/consensus/log_util.h: http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/consensus/log_util.h@208 PS1, Line 208: Status ReadEntries(std::vector>* entries); You could use LogEntries here too, if you're willing to define it outside of log-test-base.h. http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/11032/1/src/kudu/tablet/tablet_bootstrap.cc@473 PS1, Line 473: //string debug_str = entry ? SecureShortDebugString(*entry) : ""; Remove this? -- To view, visit http://gerrit.cloudera.org:8080/11032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie39516b09e4c756e8af1d8e1a5604672c96a80cb Gerrit-Change-Number: 11032 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 18:15:11 + Gerrit-HasComments: Yes
[kudu-CR] [gradle] Add rerunTests property
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11035 Change subject: [gradle] Add rerunTests property .. [gradle] Add rerunTests property [gradle] Add rerunTests property Gradle keeps track of changes and will skip tasks that are up-to-date when possible. This is great for incremental build speed, but can work against you if you want to rerun tests multiple times. Gradle does have a —rerun-tasks flag that will make sure the tests are run. However, it will also rerun all the tasks in the build tree. This patch adds a rerunTests property that will tell gradle the tests need to be rerun when passed. Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c --- M java/gradle/tests.gradle 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/11035/1 -- To view, visit http://gerrit.cloudera.org:8080/11035 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I88eebdc34c5b9763b87879eade3dc9b42d3caf1c Gerrit-Change-Number: 11035 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] memrowset: support iteration with is deleted virtual column
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10990 ) Change subject: memrowset: support iteration with is_deleted virtual column .. Patch Set 5: Verified+1 Overriding Jenkins, unrelated Java test failures. -- To view, visit http://gerrit.cloudera.org:8080/10990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 Gerrit-Change-Number: 10990 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 18:01:13 + Gerrit-HasComments: No
[kudu-CR] schema: add is deleted virtual column
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10968 ) Change subject: schema: add is_deleted virtual column .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltamemstore: support iteration with snap to exclude
Hello Tidy Bot, Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11029 to look at the new patch set (#2). Change subject: deltamemstore: support iteration with snap_to_exclude .. deltamemstore: support iteration with snap_to_exclude This commit modifies the DeltaMemStore iterator to take snap_to_exclude into consideration (if it is set) when determining which updates and deletes are relevant to an iterator. I also copied the no-reinsert DCHECK into Update(), as I wasted some time adding reinserts to a test only to learn later (when I added iteration) that the DMS doesn't allow that. Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 --- M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc 2 files changed, 145 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/11029/2 -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: support iteration with include deleted rows
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10929 ) Change subject: memrowset: support iteration with include_deleted_rows .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/10929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ie018043518b437ecc719cf9f87b2c5eea560c9a1 Gerrit-Change-Number: 10929 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] schema: add is deleted virtual column
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10968 ) Change subject: schema: add is_deleted virtual column .. Patch Set 7: Verified+1 Overriding Jenkins, unrelated Java test failures. -- To view, visit http://gerrit.cloudera.org:8080/10968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56eb1d44ba8bfbd76d8bb794b8076b695782939e Gerrit-Change-Number: 10968 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 18:01:05 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with include deleted rows
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10929 ) Change subject: memrowset: support iteration with include_deleted_rows .. Patch Set 5: Verified+1 Overriding Jenkins, unrelated Java test failures. -- To view, visit http://gerrit.cloudera.org:8080/10929 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie018043518b437ecc719cf9f87b2c5eea560c9a1 Gerrit-Change-Number: 10929 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 18:00:57 + Gerrit-HasComments: No
[kudu-CR] memrowset: support iteration with is deleted virtual column
Adar Dembo has removed Dan Burkert from this change. ( http://gerrit.cloudera.org:8080/10990 ) Change subject: memrowset: support iteration with is_deleted virtual column .. Removed reviewer Dan Burkert. -- To view, visit http://gerrit.cloudera.org:8080/10990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ic6b053f5a3696eb9d7c26b8e3d96752f4f87bcd8 Gerrit-Change-Number: 10990 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] memrowset: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 ) Change subject: memrowset: support iteration with snap_to_exclude .. Patch Set 7: Verified+1 Overriding Jenkins, unrelated Java test failures. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Jul 2018 18:00:45 + Gerrit-HasComments: No
