Dan Burkert has posted comments on this change. Change subject: KUDU-861 Support changing default, storage attributes ......................................................................
Patch Set 6: (3 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); > Yeah, but that's not a good excuse for me not to at least try and improve i After thinking about this a bit more (and hitting a similar issue with the IN list stuff), I'm not sure how it could be realistically improved. Adding a KuduValue type interface doesn't really make the situation any better; there is still the opportunity for a type mismatch with the column. Probably best to leave as-is for now. http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/client/table_alterer-internal.cc File src/kudu/client/table_alterer-internal.cc: Line 106: RETURN_NOT_OK(s.spec->ToColumnSchemaDelta(&col_delta)); This does some of the checks above, right? Maybe it would be best to consolidate all of them in ToColumnSchemaDelta. 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 > Done. Is there a goal to remove gscoped_ptr in favor of unique_ptr througho Yes, I think long term. It's hairy to update that much code at once, though. For now we are just trying to not introduce more usage of gscoped_ptr. -- 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
