[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 12: Code-Review+2 As we discussed, please loop the relevant tests for a while to see if there are any residual failures. -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 12: Build Started http://104.196.14.100/job/kudu-gerrit/2612/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#12). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 19 files changed, 1,180 insertions(+), 208 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/12 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3648/11/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1594: if (has_metadata_changes) {
What about this? Shouldn't it be the same condition as when setting
actions.table_to_update?
--
To view, visit http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Dan Burkert has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3648/9/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1522: if (has_metadata_changes) {
> But what if !tablets_to_add.empty() and !has_metadata_changes_for_existing_
Updated. I opted to keep the behavior as not incrementing the version on an
alter operation that only adds or removes tablets, since having the table be in
ALTER state multiple times for a given version does not seem to cause any
issues.
--
To view, visit http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 11: Build Started http://104.196.14.100/job/kudu-gerrit/2586/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#11). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 19 files changed, 1,180 insertions(+), 208 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/11 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3648/9/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1522: if (has_metadata_changes) {
> Yes, that's basically what 'has_metadata_changes' is tracking.
But what if !tablets_to_add.empty() and
!has_metadata_changes_for_existing_tablets? Seems like that could happen if an
AlterTable() comes in that just adds a new partition. In that case, we will set
the table's state to ALTERING but not write it out, and revert the change
below. That doesn't seem correct.
--
To view, visit http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Dan Burkert has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1345: }
> I think you missed this from earlier (as did I when I gave you a +2).
Storing crefs isn't possible, but I've updated this to just make copies. I'm a
little surprised it compiled earlier since it clearly broke the const invariant
on the request pointer.
http://gerrit.cloudera.org:8080/#/c/3648/8/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1417: string table_name = l.data().name();
> This can now be removed.
Done
http://gerrit.cloudera.org:8080/#/c/3648/9/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1522: if (has_metadata_changes) {
> Shouldn't this run in any case where l.mutable_data() was modified?
Yes, that's basically what 'has_metadata_changes' is tracking.
--
To view, visit http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 10: Build Started http://104.196.14.100/job/kudu-gerrit/2572/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#10). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 19 files changed, 1,178 insertions(+), 208 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/10 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3648/9/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1522: if (has_metadata_changes) {
Shouldn't this run in any case where l.mutable_data() was modified?
Line 1593: if (has_metadata_changes) {
This too.
--
To view, visit http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#9). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 19 files changed, 1,172 insertions(+), 194 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/9 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 9: Build Started http://104.196.14.100/job/kudu-gerrit/2570/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 1345: } > Can't these vectors be of crefs instead? I just don't think any other metho I think you missed this from earlier (as did I when I gave you a +2). http://gerrit.cloudera.org:8080/#/c/3648/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 1417: bool has_changes = false; This can now be removed. -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 8: Build Started http://104.196.14.100/job/kudu-gerrit/2554/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#8). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 19 files changed, 1,169 insertions(+), 190 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/8 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 7: Build Started http://104.196.14.100/job/kudu-gerrit/2548/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Dan Burkert has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 6:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS6, Line 853: KuduPartialRow* lower_bound,
:
KuduPartialRow* upper_bound
> Should we first check that the bounds aren't null?
Done
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 406: Status WaitForMasterUpAndRunning() WARN_UNUSED_RESULT;
> Maybe doc this new method? And since it's an ExternalMaster method, maybe r
Done
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS6, Line 1267: return Status::NotFound("New partition conflicts
with existing partition",
: step.DebugString());
> Can we point out the conflict in these messages? Or is that too verbose?
It will be pretty verbose, and I don't think it's too useful at this point
(experience may prove me wrong, though).
PS6, Line 1272: auto prev = std::prev(existing_iter);
: TabletMetadataLock metadata(prev->second,
TabletMetadataLock::READ);
> Nit: combine?
Done
Line 1284: if (metadata.pb.partition().partition_key_start()<
upper_bound) {
> Nit: partition_key_start() < upper_bound
Done
Line 1331: existing_tablets.erase(existing_iter);
> Maybe CHECK that this returns 1? Or add EraseOrDie to map-util.h and use th
This is erasing by iterator, not by key, so it can't fail (and doesn't return
an int).
Line 1551: {
> Please add a comment here rationalizing the order of operations.
Done
Line 1563: table->AddRemoveTablets(tablets_to_add, tablets_to_drop);
> I don't remember why we thought this should be done with the global catalog
Done
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 207: // Adds all tablets to the return vector in partition key sorted
order.
> Nit: returned vector
I really meant 'return' here, although I see how it's confusing, so I just
removed 'return' altogether.
Line 230: std::map tablet_map() const {
> Let's make the TabletInfoMap typedef public and use it in this method.
Done
--
To view, visit http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#7). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 19 files changed, 1,164 insertions(+), 191 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/7 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 6:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS6, Line 853: KuduPartialRow* lower_bound,
:
KuduPartialRow* upper_bound
Should we first check that the bounds aren't null?
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/integration-tests/external_mini_cluster.h
File src/kudu/integration-tests/external_mini_cluster.h:
Line 406: Status WaitForMasterUpAndRunning() WARN_UNUSED_RESULT;
Maybe doc this new method? And since it's an ExternalMaster method, maybe
rename to "WaitForCatalogManager" or something more specific?
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 1345: }
> yes, otherwise the messages would need to be copied.
Can't these vectors be of crefs instead? I just don't think any other methods
here modify the input, and it doesn't seem like something we should do.
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS6, Line 1267: return Status::NotFound("New partition conflicts
with existing partition",
: step.DebugString());
Can we point out the conflict in these messages? Or is that too verbose?
PS6, Line 1272: auto prev = std::prev(existing_iter);
: TabletMetadataLock metadata(prev->second,
TabletMetadataLock::READ);
Nit: combine?
Line 1284: if (metadata.pb.partition().partition_key_start()<
upper_bound) {
Nit: partition_key_start() < upper_bound
Line 1331: existing_tablets.erase(existing_iter);
Maybe CHECK that this returns 1? Or add EraseOrDie to map-util.h and use that?
Line 1551: {
Please add a comment here rationalizing the order of operations.
Line 1563: table->AddRemoveTablets(tablets_to_add, tablets_to_drop);
I don't remember why we thought this should be done with the global catalog
manager spinlock held. Can you remind me?
http://gerrit.cloudera.org:8080/#/c/3648/6/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
Line 207: // Adds all tablets to the return vector in partition key sorted
order.
Nit: returned vector
Line 230: std::map tablet_map() const {
Let's make the TabletInfoMap typedef public and use it in this method.
--
To view, visit http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 6: -Verified Build Started http://104.196.14.100/job/kudu-gerrit/2542/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Dan Burkert has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 4:
(37 comments)
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS4, Line 892:
RETURN_NOT_OK(data_->client_->data_->WaitForAlterTableToFinish(
: data_->client_, alter_name, deadline));
> If this fails, don't we want to clear the meta cache anyway? Maybe ClearCac
It can actually happen before waiting for the reasons outlined below. Moved.
Line 908: // It is sufficient to clear that cache even if the table
alteration is not
> Nit: "the cache".
Done
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.h
File src/kudu/client/client.h:
PS4, Line 537: unless the existing range partitions are dropped first
> What happens if a single KuduTableAlterer has DropRange and AddRange on the
DropRange then AddRange is legal if the range already exists, and effectively
truncates the existing range, but the new range is still subject to the
visibility caveats. AddRange then DropRange is legal if the range doesn't
already exist, and is effectively a no-op (the tablet shouldn't even be
created). And finally, any combination of these is possible, e.g. Add -> Drop
-> Add -> Drop, etc.
Line 541: // this method returns, however other existing clients may have to
wait for a
> Don't you mean "when Alter() returns success"?
Done
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:
Line 19:
> What were the include and using changes for?
mistake.
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/table_alterer-internal.h
File src/kudu/client/table_alterer-internal.h:
Line 57: struct PartitioningStep {
> I think the code would be net less complex if PartitionStep and Step were c
Done
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
Line 363: LOG(INFO) << "Adding column " << name;
> Were these LOG statements just for debugging or do you want to keep them?
They are really helpful in debugging the test, and they don't happen too often
(insert/update/delete are common, but set to vlog).
Line 459: scanner.SetTimeoutMillis(6);
> What's the significance of this value?
This was copied from ScanTableToStrings, which wasn't suitable anymore because
it doesn't use a fault tolerant scanner (sort order is important).
Line 510: vector master_addrs_;
> Unused?
Done
PS4, Line 547: } else if (r < 850 && t.num_range_partitions() <
kMaxRangePartitions) {
: t.AddRangePartition();
: } else if (r < 900) {
: t.DropRangePartition();
> It would be cool if this also tested "compound" operations, i.e. an AlterTa
Done
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
Line 27: #include "kudu/client/client.h"
> Nit: the previous sort order was more correct.
Done
Line 36: #include "kudu/master/master-test-util.h"
> Nit: likewise, this was more correct before.
Done
Line 463: session->SetTimeoutMillis(15 * 1000);
> Why this value?
It's copied from InsertRows above.
Line 468: gscoped_ptr insert(table->NewInsert());
> Use unique_ptr here?
Done
PS4, Line 470: if (table->schema().num_columns() > 1) {
: RETURN_NOT_OK(insert->mutable_row()->SetInt32(1, i));
: }
> The assumption being that every column is going to be an Int32?
Yes, this method assumes the test class's schema_ as the schema.
Line 550: int AlterTableTest::CountRows(const string& table_name) {
> Can you reuse CountTableRows() from client-test-util.h? There may be some o
Done
Line 1064: TEST_F(AlterTableTest, TestAlterRangePartitioning) {
> How about a test case for one AlterTable() adding and dropping the same par
Done
Line 1065: gscoped_ptr table_alterer;
> unique_ptr?
Done
PS4, Line 1091: table_alterer->wait(true);
:
ASSERT_OK(table_alterer->AddRangePartition(add_lower.release(),
add_upper.release())->Alter());
> Combine?
Done
PS4, Line 1097: ASSERT_OK(InsertRowsSequential(table_name, 0, 100));
: ASSERT_EQ(100, CountRows(table_name));
> This is to test that IsAlterTableInProgress()->false actually means we can
yah, I changed this up slightly since that's not really worth testing as it was.
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS4, Line 1215: Schema schema;
:
RETURN_NOT_OK(SchemaFromPB(table->metadata().state().pb.schema(), &schema));
: PartitionSchema partition_schema;
:
RETURN_NOT_
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 6: Build Started http://104.196.14.100/job/kudu-gerrit/2531/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#6). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/external_mini_cluster.cc M src/kudu/integration-tests/external_mini_cluster.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 19 files changed, 1,135 insertions(+), 191 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/6 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/2493/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Adar Dembo has posted comments on this change.
Change subject: KUDU-1311 [master] support adding and dropping range partitions
..
Patch Set 4:
(37 comments)
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS4, Line 892:
RETURN_NOT_OK(data_->client_->data_->WaitForAlterTableToFinish(
: data_->client_, alter_name, deadline));
If this fails, don't we want to clear the meta cache anyway? Maybe ClearCache()
should be set up as a ScopedCleanup thing right after L888-889.
Line 908: // It is sufficient to clear that cache even if the table
alteration is not
Nit: "the cache".
Also, not sure about "sufficient"; it implies that there was something more we
could do but have chosen not to. Maybe you meant "necessary"?
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.h
File src/kudu/client/client.h:
PS4, Line 537: unless the existing range partitions are dropped first
What happens if a single KuduTableAlterer has DropRange and AddRange on the
same range? Is that legal, provided they're in that order?
Line 541: // this method returns, however other existing clients may have to
wait for a
Don't you mean "when Alter() returns success"?
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:
Line 19:
What were the include and using changes for?
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/table_alterer-internal.h
File src/kudu/client/table_alterer-internal.h:
Line 57: struct PartitioningStep {
I think the code would be net less complex if PartitionStep and Step were
combined. The overhead of two unique_ptrs per Step for non-partitioning steps
is minimal, and it'll simplify ToRequest() somewhat.
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:
Line 363: LOG(INFO) << "Adding column " << name;
Were these LOG statements just for debugging or do you want to keep them?
Line 459: scanner.SetTimeoutMillis(6);
What's the significance of this value?
Line 510: vector master_addrs_;
Unused?
PS4, Line 547: } else if (r < 850 && t.num_range_partitions() <
kMaxRangePartitions) {
: t.AddRangePartition();
: } else if (r < 900) {
: t.DropRangePartition();
It would be cool if this also tested "compound" operations, i.e. an
AlterTable() that adds a partition, drops another, and adds a column too.
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
Line 27: #include "kudu/client/client.h"
Nit: the previous sort order was more correct.
Line 36: #include "kudu/master/master-test-util.h"
Nit: likewise, this was more correct before.
Line 463: session->SetTimeoutMillis(15 * 1000);
Why this value?
Line 468: gscoped_ptr insert(table->NewInsert());
Use unique_ptr here?
PS4, Line 470: if (table->schema().num_columns() > 1) {
: RETURN_NOT_OK(insert->mutable_row()->SetInt32(1, i));
: }
The assumption being that every column is going to be an Int32?
Line 550: int AlterTableTest::CountRows(const string& table_name) {
Can you reuse CountTableRows() from client-test-util.h? There may be some other
useful goodies there.
Line 1064: TEST_F(AlterTableTest, TestAlterRangePartitioning) {
How about a test case for one AlterTable() adding and dropping the same
partition?
Also, what about negative test cases? Like dropping partitions that don't
exist, adding partitions that overlap with existing ones, adding the same
partition twice, etc.
Line 1065: gscoped_ptr table_alterer;
unique_ptr?
PS4, Line 1091: table_alterer->wait(true);
:
ASSERT_OK(table_alterer->AddRangePartition(add_lower.release(),
add_upper.release())->Alter());
Combine?
PS4, Line 1097: ASSERT_OK(InsertRowsSequential(table_name, 0, 100));
: ASSERT_EQ(100, CountRows(table_name));
This is to test that IsAlterTableInProgress()->false actually means we can
insert/scan?
http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS4, Line 1215: Schema schema;
:
RETURN_NOT_OK(SchemaFromPB(table->metadata().state().pb.schema(), &schema));
: PartitionSchema partition_schema;
:
RETURN_NOT_OK(PartitionSchema::FromPB(table->metadata().state().pb.partition_schema(),
: schema,
&partition_schema));
This isn't safe; you should be getting the schema() through the COWLocked
object.
PS4, Line 1223: std::lock_guard l(table->lock_);
: tablets = table->tablet_map_;
This is w
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#5). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 17 files changed, 713 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/5 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#4). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 17 files changed, 713 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/4 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/2438/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Dan Burkert has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/3648/3/src/kudu/client/client.h File src/kudu/client/client.h: PS3, Line 546: dropped > nit Done -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/3648/3/src/kudu/client/client.h File src/kudu/client/client.h: PS3, Line 546: dropped nit -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/2437/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#3). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 17 files changed, 716 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/3 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Dan Burkert has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/3648/1/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 900: // If there are add partition steps, clear the local meta cache so that the > Not sure I like this, it's pretty heavy handed to clear the whole cache. Al Added a comment about why clearing the cache is still effective even if the alter is not waited on. As far as the perf. impact of this change, I don't expect it to happen very often. I also agree that most instances will be as part of something like a CLI tool that has a fresh client anyway. For applications like kudu-ts which have long running clients which may occasionally add partitions, I don't think the perf. impact will be that meaningful. We can always revisit if it ends up being a problem. http://gerrit.cloudera.org:8080/#/c/3648/1/src/kudu/client/client.h File src/kudu/client/client.h: PS1, Line 550: // Multiple range partitions may be dropped, but they must all exatly match : // one of the tables range partitions. > So this means that you can drop multiple tablets? So it's not just dropping No, this would happen if the method is called multiple times as part of a single alter operation. -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2435/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3648 to look at the new patch set (#2). Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 17 files changed, 712 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/2 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Jean-Daniel Cryans has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/3648/1/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 900: // If there are add partition steps, clear the local meta cache so that the Not sure I like this, it's pretty heavy handed to clear the whole cache. Also, if you don't wait above, won't the tablets still not be created and we might end up re-caching non-covered ranges? OTOH I don't think it'd be pretty common to alter from an application that manipulates data, unless it's like a shell. http://gerrit.cloudera.org:8080/#/c/3648/1/src/kudu/client/client.h File src/kudu/client/client.h: PS1, Line 550: // Multiple range partitions may be dropped, but they must all exatly match : // one of the tables range partitions. So this means that you can drop multiple tablets? So it's not just dropping one partition? -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Dan Burkert has uploaded a new change for review. http://gerrit.cloudera.org:8080/3648 Change subject: KUDU-1311 [master] support adding and dropping range partitions .. KUDU-1311 [master] support adding and dropping range partitions Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/table_alterer-internal.cc M src/kudu/client/table_alterer-internal.h M src/kudu/common/partition.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/ksck.cc 17 files changed, 698 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/3648/1 -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert
[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions
Kudu Jenkins has posted comments on this change. Change subject: KUDU-1311 [master] support adding and dropping range partitions .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2429/ -- To view, visit http://gerrit.cloudera.org:8080/3648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
