Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/2455/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1368: // If this this update result contains a catalog objects to add or remove, directly apply > (nit again) "a" can be removed. Done Line 1368: // If this this update result contains a catalog objects to add or remove, directly apply > Not your change but "this" is duplicated. (nit) Done http://gerrit.cloudera.org:8080/#/c/2455/1/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: Line 53: // This field is superceded by the updated_catalog_objects list below, but is still > (nit) I think there are some trailing whitespaces here and twice in the fol Done http://gerrit.cloudera.org:8080/#/c/2455/1/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 299: // Check that the response either exclusively uses the single updated/removed field > Just try to understand the solution. do we expect older client that uses si Yes, the lists field was added for persistent Java UDFs. Older clients expect the single field to be set. Like you said, the change is to only set the lists field when required. Line 897: } > I think we should do something like Good catch, you are right. Done, Line 1279: } > I think this should be done similar to my comment on createFunction() Good catch, you are right. Done, -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-HasComments: Yes
