[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions

2016-07-21 Thread Adar Dembo (Code Review)
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

2016-07-21 Thread Kudu Jenkins (Code Review)
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

2016-07-21 Thread Dan Burkert (Code Review)
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

2016-07-20 Thread Adar Dembo (Code Review)
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

2016-07-20 Thread Kudu Jenkins (Code Review)
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

2016-07-20 Thread Dan Burkert (Code Review)
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

2016-07-20 Thread Adar Dembo (Code Review)
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

2016-07-20 Thread Dan Burkert (Code Review)
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

2016-07-20 Thread Kudu Jenkins (Code Review)
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

2016-07-20 Thread Dan Burkert (Code Review)
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

2016-07-20 Thread Adar Dembo (Code Review)
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

2016-07-20 Thread Dan Burkert (Code Review)
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

2016-07-20 Thread Kudu Jenkins (Code Review)
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

2016-07-19 Thread Adar Dembo (Code Review)
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

2016-07-19 Thread Kudu Jenkins (Code Review)
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

2016-07-19 Thread Dan Burkert (Code Review)
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

2016-07-19 Thread Adar Dembo (Code Review)
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

2016-07-19 Thread Kudu Jenkins (Code Review)
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

2016-07-19 Thread Dan Burkert (Code Review)
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

2016-07-19 Thread Dan Burkert (Code Review)
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

2016-07-19 Thread Adar Dembo (Code Review)
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

2016-07-19 Thread Kudu Jenkins (Code Review)
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

2016-07-18 Thread Dan Burkert (Code Review)
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(), ));
  :   PartitionSchema partition_schema;
  :   

[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions

2016-07-18 Thread Kudu Jenkins (Code Review)
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

2016-07-14 Thread Kudu Jenkins (Code Review)
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

2016-07-14 Thread Adar Dembo (Code Review)
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(), ));
  :   PartitionSchema partition_schema;
  :   
RETURN_NOT_OK(PartitionSchema::FromPB(table->metadata().state().pb.partition_schema(),
  : schema, 
_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 why you needed to 

[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions

2016-07-14 Thread Dan Burkert (Code Review)
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

2016-07-14 Thread Kudu Jenkins (Code Review)
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

2016-07-14 Thread Dan Burkert (Code Review)
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

2016-07-14 Thread Dan Burkert (Code Review)
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

2016-07-14 Thread Kudu Jenkins (Code Review)
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

2016-07-14 Thread Dan Burkert (Code Review)
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

2016-07-14 Thread Jean-Daniel Cryans (Code Review)
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

2016-07-13 Thread Dan Burkert (Code Review)
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

2016-07-13 Thread Kudu Jenkins (Code Review)
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