[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Internal Jenkins has submitted this change and it was merged. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. While adding support for permanent UDFs we made incompatible changes to the CatalogService Thrift definitions. Some services like BDR rely on a stable catalog API. This patch fixes the incompatibility. Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Reviewed-on: http://gerrit.cloudera.org:8080/2455 Reviewed-by: Alex Behm Tested-by: Internal Jenkins --- M be/src/service/impala-server.cc M common/thrift/CatalogService.thrift M common/thrift/Types.thrift M fe/src/main/java/com/cloudera/impala/catalog/Function.java M fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java M tests/catalog_service/test_catalog_service_client.py 6 files changed, 114 insertions(+), 38 deletions(-) Approvals: Internal Jenkins: Verified Alex Behm: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Silvius Rus
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 6: Code-Review+2 fixed a typo -- 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: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Silvius Rus Gerrit-HasComments: No
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Hello Henry Robinson, Internal Jenkins, Silvius Rus, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/2455 to look at the new patch set (#6). Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. While adding support for permanent UDFs we made incompatible changes to the CatalogService Thrift definitions. Some services like BDR rely on a stable catalog API. This patch fixes the incompatibility. Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad --- M be/src/service/impala-server.cc M common/thrift/CatalogService.thrift M common/thrift/Types.thrift M fe/src/main/java/com/cloudera/impala/catalog/Function.java M fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java M tests/catalog_service/test_catalog_service_client.py 6 files changed, 114 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/55/2455/6 -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Silvius Rus
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Hello Henry Robinson, Silvius Rus, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/2455 to look at the new patch set (#5). Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. While adding support for permanent UDFs we made incompatible changes to the CatalogService Thrift definitions. Some services like BDR rely on a stable catalog API. This patch fixes the incompatibility. Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad --- M be/src/service/impala-server.cc M common/thrift/CatalogService.thrift M common/thrift/Types.thrift M fe/src/main/java/com/cloudera/impala/catalog/Function.java M fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java M tests/catalog_service/test_catalog_service_client.py 6 files changed, 114 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/55/2455/5 -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Silvius Rus
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 5: Code-Review+2 Added TODO for tests as discussed. Rebased. -- 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: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Silvius Rus Gerrit-HasComments: No
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Silvius Rus has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 4: Code-Review+2 Approved by gatekeeper. Please add a TODO for the missing legacy interface tests. We can check in tests later on as long as they don't require absolutely any code changes. Also, this will be covered by BDR tests. -- 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: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Silvius Rus Gerrit-HasComments: No
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Henry Robinson has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 4: Code-Review+1 Do you need to add a test for the legacy interface? -- 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: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: No
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has uploaded a new patch set (#4). Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. While adding support for permanent UDFs we made incompatible changes to the CatalogService Thrift definitions. Some services like BDR rely on a stable catalog API. This patch fixes the incompatibility. Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad --- M be/src/service/impala-server.cc M common/thrift/CatalogService.thrift M common/thrift/Types.thrift M fe/src/main/java/com/cloudera/impala/catalog/Function.java M fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java 5 files changed, 111 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/55/2455/4 -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/2455/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1398: return Status(err.str()); > use TStatusCode::INTERNAL_ERROR ? Done http://gerrit.cloudera.org:8080/#/c/2455/3/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 300: // or the corresponding list versions of the fields, but not a mix. > I think the check doesn't handle the case where neither are set. You don't Not all responses have updated/removed objects. For example, CREATE TABLE IF NOT EXISTS will return success but no updated object if the table already exists. I changed the comment to clarify. Line 897: // on a stable catalog Thrift API. > Is it always the case that the new use of the API will return > 1 function? No, not always the case. Changed to only use those result fields for persistent Java fns. Made the same change for drop fn. -- 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: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Henry Robinson has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/2455/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1398: Status(err.str() use TStatusCode::INTERNAL_ERROR ? http://gerrit.cloudera.org:8080/#/c/2455/3/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 299: either exclusively uses the single updated/removed field : // or the corresponding list versions of the fields, but not a mix. I think the check doesn't handle the case where neither are set. You don't have to handle it (although it might be worthwhile), but this comment makes it sounds like it does check that condition. Line 895: Distinguish which result field to set based on the number of added : // functions for backwards compatibility. For example, BDR relies : // on a stable catalog Thrift API. Is it always the case that the new use of the API will return > 1 function? -- 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: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has uploaded a new patch set (#3). Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. While adding support for permanent UDFs we made incompatible changes to the CatalogService Thrift definitions. Some services like BDR rely on a stable catalog API. This patch fixes the incompatibility. Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad --- M be/src/service/impala-server.cc M common/thrift/CatalogService.thrift M common/thrift/Types.thrift M fe/src/main/java/com/cloudera/impala/catalog/Function.java M fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java 5 files changed, 102 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/55/2455/3 -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 2: (1 comment) 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); > I'm not sure I agree. Now every API (except one) uses the legacy interface. A prerequisite for that would be a 100% clear understanding of the cases in which we need to preserve compatibility (so we can use the new API for the other cases). -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Henry Robinson has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 2: (1 comment) 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); > Technically we don't have to for the BDR problem because that is specific t I'm not sure I agree. Now every API (except one) uses the legacy interface. Don't we want to standardise on the new interface as soon as possible? It seems better to have the legacy use be the exception, not the norm. -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Henry Robinson has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 2: (5 comments) It looks as though this patch returns everything to use single objects in the response type, except for the function case where the caller uses the new 'api'. Is that necessary, or defensive against other issues of this type? http://gerrit.cloudera.org:8080/#/c/2455/2//COMMIT_MSG Commit Message: Line 10: servies services 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 down debug clusters? Better to return a response with an explanation. 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 affect BDR, but we will be able to immediately see any new code that relies on this. Line 60: removed_catalog_object ditto for naming 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 go back to singletons (rather than lists) for this case? -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 1: (1 comment) 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 > Got it, thanks for the explanation. FWIW, I discussed this with Henry also to get another opinion, and he agrees with the current solution - even if not completely clean. -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Juan Yu has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 1: (1 comment) 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 > Sorry, I forgot to answer your last question :) Got it, thanks for the explanation. -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 1: (1 comment) 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 > Yes, you are right. We could also add a new version of the protocol and plu Sorry, I forgot to answer your last question :) In the current implementation, clients will need to check all four fields like here. The other alternative is to add a new protocol version. -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 1: (1 comment) 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 > The implementation seems more like the latter, "don't care about client, ju Yes, you are right. We could also add a new version of the protocol and plumb that through. The bottom line though is that the CatalogService and its API is not a public service. Nobody should be writing against that API except Impala. -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Juan Yu has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 1: (1 comment) 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 > Yes, the lists field was added for persistent Java UDFs. Older clients expe The implementation seems more like the latter, "don't care about client, just set single field if there is one object, and set list field if there are more than one object." If I write a new client, I need to check all four fields, right? -- 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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has uploaded a new patch set (#2). Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. While adding support for permanent UDFs we made incompatible changes to the CatalogService Thrift definitions. Some servies like BDR rely on a stable catalog API. This patch fixes the incompatibility. Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad --- M be/src/service/impala-server.cc M common/thrift/CatalogService.thrift M common/thrift/Types.thrift M fe/src/main/java/com/cloudera/impala/catalog/Function.java M fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java 5 files changed, 89 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/55/2455/2 -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Juan Yu has posted comments on this change. Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. Patch Set 1: (1 comment) 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: exclusively Just try to understand the solution. do we expect older client that uses single field and newer client that uses list field? or we don't care about client, just set single field if there is one object, and set list field if there are more than one object. -- 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 Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
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
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-HasComments: Yes
[Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Alex Behm has uploaded a new change for review. http://gerrit.cloudera.org:8080/2455 Change subject: OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. .. OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible. While adding support for permanent UDFs we made incompatible changes to the CatalogService Thrift definitions. Some servies like BDR rely on a stable catalog API. This patch fixes the incompatibility. Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad --- M be/src/service/impala-server.cc M common/thrift/CatalogService.thrift M common/thrift/Types.thrift M fe/src/main/java/com/cloudera/impala/catalog/Function.java M fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java 5 files changed, 73 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/55/2455/1 -- To view, visit http://gerrit.cloudera.org:8080/2455 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iec04d07c48d7159d2837667d7039046de126a3ad Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm
