Dan Burkert has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes ......................................................................
Patch Set 6: (10 comments) 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 following the lead of the existing default value stuff in Java. 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. 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 clause. Resetting a null shared_ptr should be fine. Line 82: if (read_default_ == NULL) { same here. Line 96: if (write_default_ == NULL) { and here Line 105: if (write_default_ == NULL) { and here 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 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 it_names = col_names_.find(col_delta.name) to the line above, then just check if it_names == col_names_.end() 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. 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. -- 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
