Bharath Vissapragada has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/2455/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1368: this this Not your change but "this" is duplicated. (nit) Line 1368: a (nit again) "a" can be removed. http://gerrit.cloudera.org:8080/#/c/2455/1/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: Line 53: (nit) I think there are some trailing whitespaces here and twice in the following lines. 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 894: if (!addedFunctions.isEmpty()) { : resp.result.setUpdated_catalog_objects(addedFunctions); : resp.result.setVersion(catalog_.getCatalogVersion()); : } I think we should do something like if (addedFunctions.size() == 1) setUpdated_catalog_object() else if( setUpdated_catalog_objects > 1) setUpdated_catalog_objects() so that this doesn't break BDR if they call CREATE_FUNCTION with old thrift API. Line 1277: if (removedFunctions.size() > 0) { : resp.result.setRemoved_catalog_objects(removedFunctions); : } I think this should be done similar to my comment on createFunction() -- 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: Bharath Vissapragada <[email protected]> Gerrit-HasComments: Yes
