impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Huaisi Xu (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Date Thu, 30 Jun 2016 18:20:26 GMT
Huaisi Xu has posted comments on this change.

Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................


Patch Set 13:

(11 comments)

thanks Henry and Dimitris

http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:

PS13, Line 175: if (matcher == null) return Collections.emptyList();
> rather than dealing with this case everywhere, push the logic into PatternM
Setting the pattern of the matcher to null is the problem we are trying to solve.

I can push the logic to filterCatalogObjectsByPattern and filterStringsByPattern though. what
do you think


PS13, Line 226: See filterStringsByPattern
              :    * for details of the pattern match semantics.
> is this comment stale now?
Done


PS13, Line 231: getDataSourceNames
> Is this version really necessary?  We don't have this specialisation for ge
what do you mean by version? I changed this code because I changed the interface of filterStringsByPattern
to take a pattern matcher?


PS13, Line 237: aal
> all
Done


PS13, Line 237: the pattern of 'matcher'.
> nit, just say 'that match 'matcher''
Done


PS13, Line 238: @see PatternMatcher for supported matchers
> this seems redundant, and can be removed.
Done, and I changed all occurrence.


PS13, Line 327: If
              :    * 'matcher' does not have a pattern, all catalog objects in 'candidates'
are returned.
> This comment leaks through the abstraction of PatternMatcher - i.e. it shou
Done


PS13, Line 334: private
> probably should live in PatternMatcher itself.
then the same may apply to filterCatalogObjectsByPattern(), but it accesses getName(). So
I guess it is better to take a filterKey functor as input? so it can call getName() accordingly?
But I think that is unnecessary refactor? what do you think?


PS13, Line 360: f
              :    * 'matcher' does not have a pattern, all catalog objects in 'candidates'
are returne
> same comment as above
Done


PS13, Line 365: @see PatternMatcher#matches(String) for matching mechanisms.
> I don't think this is necessary. Callers of this method will have construct
Done


http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:

PS13, Line 274:  public boolean hasDbPattern() { return isDbPatternSet_; }
              :     public boolean hasTablePattern() { return isTablePatternSet_; }
              :     public boolean hasColumnPattern() { return isColumnPatternSet_; }
              :     public boolean hasFunctionPattern() { return isFunctionPatternSet_; }
> it would be cleaner to make it so that the default values (maybe null?) do 
null is a valid pattern. so we cannot tell if a null pattern is set by the caller or it is
just unset. This is one of the problem we are trying to solve.


-- 
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hxu@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hxu@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message