Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RATIS-2133. Ignore http proxy for grpc client & server #1128

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

sadanand48
Copy link
Contributor

@sadanand48 sadanand48 commented Aug 1, 2024

What changes were proposed in this pull request?

Referring to this comment , Ratis should not use http proxy while setting up grpc/netty clients if configured.

Reference : grpc/grpc-java#5600 (comment)

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-2133

How was this patch tested?

Verified by building & replacing the ratis-grpc jar and trying out an Ozone write with jvm args set for http proxy.

Also verified via unit test using

mvn -Dtest=TestLeaderElectionWithGrpc \
    -Dhttp.proxyHost=127.0.0.1 -Dhttp.proxyPort=8080 -Dhttps.proxyHost=127.0.0.1 -Dhttps.proxyPort=8080 -Djava.net.useSystemProxies=true \
    clean test

Before the fix:

incubator-ratis % cat ratis-test/target/surefire-reports/org.apache.ratis.grpc.TestLeaderElectionWithGrpc-output.txt | grep "NoClass" | head -n 1

Caused by: java.lang.NoClassDefFoundError: org/apache/ratis/thirdparty/io/netty/handler/proxy/HttpProxyHandler

After the fix:

incubator-ratis % cat ratis-test/target/surefire-reports/org.apache.ratis.grpc.TestLeaderElectionWithGrpc-output.txt | grep "NoClass" | head -n 1

incubator-ratis %

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sadanand48 for the fix, LGTM.

BTW, the problem can also be reproduced with Ratis unit tests with a small tweak like (replace placeholder with real hostname):

diff --git ratis-common/src/main/java/org/apache/ratis/util/NetUtils.java ratis-common/src/main/java/org/apache/ratis/util/NetUtils.java
index baf6762a..cd708185 100644
--- ratis-common/src/main/java/org/apache/ratis/util/NetUtils.java
+++ ratis-common/src/main/java/org/apache/ratis/util/NetUtils.java
@@ -158,7 +158,7 @@ public interface NetUtils {
     return b.append(':').append(address.getPort()).toString();
   }
 
-  String LOCALHOST = "localhost";
+  String LOCALHOST = "<replace with hostname>";
 
   static String localhostWithFreePort() {
     return LOCALHOST + ":" + getFreePort();

To see the root cause exception:

diff --git ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
index ecb4a3dc..81ce8703 100644
--- ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
+++ ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java
@@ -81,6 +81,7 @@ public abstract class LeaderElectionTests<CLUSTER extends MiniRaftCluster>
   {
     Slf4jUtils.setLogLevel(RaftServer.Division.LOG, Level.DEBUG);
     Slf4jUtils.setLogLevel(RaftClient.LOG, Level.DEBUG);
+    Slf4jUtils.setLogLevel(LeaderElection.LOG, Level.TRACE);
   }
 
   @Test

Then run:

mvn -Dtest=TestLeaderElectionWithGrpc \
    -Dhttp.proxyHost=127.0.0.1 -Dhttp.proxyPort=8080 -Dhttps.proxyHost=127.0.0.1 -Dhttps.proxyPort=8080 -Djava.net.useSystemProxies=true \
    clean test

@sadanand48
Copy link
Contributor Author

sadanand48 commented Aug 1, 2024

Thanks @adoroszlai , verified the fix by running TestLeaderElectionWithGrpc using your command too.

@adoroszlai adoroszlai changed the title RATIS-2133. Ignore http proxy for ratis grpc client & server. RATIS-2133. Ignore http proxy for grpc client & server Aug 1, 2024
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

@szetszwo szetszwo merged commit 99925e3 into apache:master Aug 1, 2024
12 checks passed
@szetszwo
Copy link
Contributor

szetszwo commented Aug 1, 2024

@sadanand48 , thanks for working on this!

@adoroszlai , thanks for reviewing and testing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants