Will Berkeley has posted comments on this change.

Change subject: KUDU-861 Support changing default, storage attributes
......................................................................


Patch Set 6:

(10 comments)

I'm going to add coverage in alter_table-randomized-test but wanted to post an 
update since it's been sitting for a few days.

http://gerrit.cloudera.org:8080/#/c/4310/6/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java:

Line 147:     ByteString defaultByteString = 
ProtobufHelper.objectToByteStringNoType(name, newDefault);
> This seems extremely fishy to me, but I see that you are basically followin
Yeah, but that's not a good excuse for me not to at least try and improve it. 
Ideas on how to do it better? 

My first thought is that there should be some KuduValue interface, with impls 
for each type, and then the responsibility for transforming into bytes for the 
wire is transferred to each KuduValue impl. Would be less fishy to me, and 
seems like adding new types would be easier, but it's prob a bunch of extra 
code.


http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/common/common.proto
File src/kudu/common/common.proto:

PS6, Line 92: required
> best practice is to make all protobuf fields optional.
Done


http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:

Line 73:       if (read_default_ == NULL) {
> I think the if/else here can be removed, and replaced with only the else cl
Done


Line 82:       if (read_default_ == NULL) {
> same here.
Done


Line 96:       if (write_default_ == NULL) {
> and here
Done


Line 105:       if (write_default_ == NULL) {
> and here
Done


PS6, Line 570: it_names = col_names_.find(col_delta.new_name)) != 
col_names_.end()
> I think this can be simplified using ContainsKey from map-util.h
Done


Line 575:   if ((it_names = col_names_.find(col_delta.name)) == 
col_names_.end()) {
> I think it would be more clear what's going on here if you move
Done


PS6, Line 591: LOG(FATAL) << "Should not reach here";
             :   return Status::IllegalState("Unable to alter column");
> Seems like one of these is appropriate, but not both.
Was following a pattern from elsewhere, but it is weird. Left just the fatal.


http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

PS6, Line 353: gscoped_ptr
> prefer using unique_ptr here and elsewhere.
Done. Is there a goal to remove gscoped_ptr in favor of unique_ptr throughout 
the codebase?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to