Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. ......................................................................
Patch Set 2: (5 comments) Yes, using the single fields is defensive, but it also makes the API evolution more understandable imo because we only use the new fields in cases where the old fields are insufficient. http://gerrit.cloudera.org:8080/#/c/2455/2//COMMIT_MSG Commit Message: Line 10: CatalogService Thrift definitions. Some servies like BDR rely on a stable > services Done http://gerrit.cloudera.org:8080/#/c/2455/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1383: DCHECK( > I don't think this should be a DCHECK - do we want malformed RPCs to bring Done http://gerrit.cloudera.org:8080/#/c/2455/2/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: Line 55: 4: optional CatalogObjects.TCatalogObject updated_catalog_object > I suggest changing the name to updated_catalog_object_DEPRECATED. Won't aff Done Line 60: 5: optional CatalogObjects.TCatalogObject removed_catalog_object > ditto for naming Done http://gerrit.cloudera.org:8080/#/c/2455/2/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1787: response.result.setUpdated_catalog_object(newTable); > This response isn't related to function creation, right? Why do we need to Technically we don't have to for the BDR problem because that is specific to UDFs (to the best of my knowledge). It felt cleaner to me to consistently use the old fields where possible, and only use the new fields where absolutely necessary. This maximizes compatibility, and understandability of the API evolution imo. -- 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: 2 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: Henry Robinson <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-HasComments: Yes
