impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-2.5.0_5.7.0) OPSAPS-32457: Fix CatalogService Thrift changes to be backwards compatible.
Date Fri, 04 Mar 2016 18:33:54 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Juan Yu <jyu@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message