neils-dev commented on a change in pull request #2945: URL: https://github.com/apache/ozone/pull/2945#discussion_r813191619
########## File path: hadoop-ozone/common/pom.xml ########## @@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd";> <dependency> <groupId>io.netty</groupId> <artifactId>netty-handler-proxy</artifactId> + </dependency> + <dependency> + <groupId>io.netty</groupId> + <artifactId>netty-tcnative-boringssl-static</artifactId> + <version>2.0.38.Final</version> <!-- See table for correct version --> Review comment: Thanks @hanishakoneru for reviewing this PR and for your comments. I've added the pom property for the netty-tcnative version to the central main pom.xml. Thanks. `<tcnative.version>2.0.38.Final</tcnative.version>` I also brought this up with @adoroszlai offline and he kindly added a comment to this PR on this. ########## File path: hadoop-ozone/common/pom.xml ########## @@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd";> <dependency> <groupId>io.netty</groupId> <artifactId>netty-handler-proxy</artifactId> + </dependency> + <dependency> + <groupId>io.netty</groupId> + <artifactId>netty-tcnative-boringssl-static</artifactId> + <version>2.0.38.Final</version> <!-- See table for correct version --> + <scope>runtime</scope> + </dependency> + <dependency> + <groupId>io.netty</groupId> + <artifactId>netty-tcnative</artifactId> + <version>2.0.38.Final</version> <!-- See table for correct version --> Review comment: The table comment indeed refers to the grpc tls jar version combinations (`grpc-netty`, `netty-handler`, `netty-tcnative-boringssl`) that work together. This is posted on the grpc github in the above link. ########## File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OmTransportFactory.java ########## @@ -34,26 +37,37 @@ OmTransport createOmTransport(ConfigurationSource source, static OmTransport create(ConfigurationSource conf, UserGroupInformation ugi, String omServiceId) throws IOException { - OmTransportFactory factory = createFactory(); + OmTransportFactory factory = createFactory(conf); return factory.createOmTransport(conf, ugi, omServiceId); } - static OmTransportFactory createFactory() throws IOException { - ServiceLoader<OmTransportFactory> transportFactoryServiceLoader = - ServiceLoader.load(OmTransportFactory.class); - Iterator<OmTransportFactory> iterator = - transportFactoryServiceLoader.iterator(); - if (iterator.hasNext()) { - return iterator.next(); - } + static OmTransportFactory createFactory(ConfigurationSource conf) + throws IOException { try { + // if configured transport class is different than the default + // Hadoop3OmTransportFactory, then check service loader for + // transport class and instantiate it + if (conf + .get(OZONE_OM_TRANSPORT_CLASS, + OZONE_OM_TRANSPORT_CLASS_DEFAULT) != + OZONE_OM_TRANSPORT_CLASS_DEFAULT) { Review comment: Right, the OmTransportFactory was hardcoded to instantiate the Hadoop3OmTransportFactory implementation when no other implementation was provided. I've updated this to using the default, `OZONE_OM_TRANSPORT_CLASS_DEFAULT`, when no other as suggested. Thanks @hanishakoneru, all changes pushed in latest commit. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
