> On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 27 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line27> > > > > I am not sure we need these two kinds of members
If we need to abstract configuration keys. Theere should be a way to get them as well. In this case we are using them to in the generating error messges which need to have appropriate configuration key. If not in this interface, we should be have another interface which expose the strings as well. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 34 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line34> > > > > conf isn't a sentry configuration - it may be hadoop configuration or > > Hive configuration. To avaoid confusion, I have removed sentry keyword. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 46 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line46> > > > > This one is unused - remove? This configuration is used when I work on connection pool. This refactored code is used by connection pool implmentation as well. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 69 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line69> > > > > Is it necessarily sentry principal? Would hdfs use sentry principal, > > for example? Yes, sentry.hdfs.service.server.principal is the configuration. In the current design sentry.hdfs.service.server.principal and sentry.service.server.principal will be same. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 84 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line84> > > > > What is the relationship between addresses above and ports? For > > example, can we have two RPC addresses and one port? Can address include > > port or not? How would we configure two servers binding to the same address > > but different port? port in RPC Addresses configured is optional. If a port is not provided for a server listed in RPC configuration, port configuration is used as a default port. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 100 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line100> > > > > As I mentioned before, we probably don't need this If we need to abstract configuration keys. Theere should be a way to get them as well. In this case we are using them to in the generating error messges which need to have appropriate configuration key. If not in this interface, we should be have another interface which expose the strings as well. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 106 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line106> > > > > probably don't need this If we need to abstract configuration keys. Theere should be a way to get them as well. In this case we are using them to in the generating error messges which need to have appropriate configuration key. If not in this interface, we should be have another interface which expose the strings as well. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > > Lines 112 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650802#file1650802line112> > > > > This looks a bit out of place here. Can this be refactored somehow so > > that it doesn't live with configuration? Even this is a configuration. It is MAP of SASL configuration. As we use same set of SASL configuration we can move it a common place. I will make that change in the commit after this refactor commit. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > > Lines 40 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650804#file1650804line40> > > > > You don't need this to be a singleton we talked about it before. I want to use polymorphic usage of SentryClientTransportConfigInterface. > On March 7, 2017, 1:17 a.m., Alexander Kolbasov wrote: > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java > > Lines 39 (patched) > > <https://reviews.apache.org/r/56954/diff/2/?file=1650805#file1650805line39> > > > > You don't need this to be a singleton we talked about it before. I want to use polymorphic usage of SentryClientTransportConfigInterface. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56954/#review168066 ----------------------------------------------------------- On March 7, 2017, 6:51 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56954/ > ----------------------------------------------------------- > > (Updated March 7, 2017, 6:51 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Mat Crocker, Vamsee > Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1639 > https://issues.apache.org/jira/browse/SENTRY-1639 > > > Repository: sentry > > > Description > ------- > > SENTRY-1639 Refactored the usage of configuration constants related to > transport These are subset of changes done for SENTRY-1592. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConfigInterface.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryClientTransportConstants.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryHDFSClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/transport/SentryPolicyClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > 4ed1361 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 03bf39e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceClientDefaultImpl.java > ee6cdf7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClientDefaultImpl.java > 2dc8af8 > > > Diff: https://reviews.apache.org/r/56954/diff/3/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
