-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
mobile: Rewrite the JNI code for XdsTestServer #33197
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
58 changes: 0 additions & 58 deletions
58
mobile/test/common/integration/xds_test_server_interface.cc
This file was deleted.
Oops, something went wrong.
42 changes: 0 additions & 42 deletions
42
mobile/test/common/integration/xds_test_server_interface.h
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
69 changes: 0 additions & 69 deletions
69
mobile/test/java/io/envoyproxy/envoymobile/engine/testing/TestJni.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
42 changes: 42 additions & 0 deletions
42
mobile/test/java/io/envoyproxy/envoymobile/engine/testing/XdsTestServerFactory.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
package io.envoyproxy.envoymobile.engine.testing; | ||
|
||
/** An xDS test server factory. */ | ||
public class XdsTestServerFactory { | ||
/** The instance of {@link XdsTestServer}. */ | ||
public static class XdsTestServer { | ||
private final long handle; // Used by the native code. | ||
private final String host; | ||
private final int port; | ||
|
||
private XdsTestServer(long handle, String host, int port) { | ||
this.handle = handle; | ||
this.host = host; | ||
this.port = port; | ||
} | ||
|
||
/** Starts the xDS server. */ | ||
public native void start(); | ||
|
||
/** Gets the xDS host. */ | ||
public String getHost() { return host; } | ||
|
||
/** Gets the xDS port. */ | ||
public int getPort() { return port; } | ||
|
||
/** | ||
* Sends a static `envoy::service::discovery::v3::DiscoveryResponse` message with the specified | ||
* cluster name. | ||
* | ||
* TODO(fredyw): Update to take a DiscoveryResponse proto. | ||
*/ | ||
public native void sendDiscoveryResponse(String clusterName); | ||
|
||
/** Shuts down the xDS server. */ | ||
public native void shutdown(); | ||
} | ||
|
||
static { System.loadLibrary("envoy_jni_xds_test_server_factory"); } | ||
|
||
/** Creates a new instance of {@link XdsTestServer}. */ | ||
public static native XdsTestServer create(); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it would be better to have the XdsTestServer as a top-level class, outside of XdsTestServerFactory? Same for the other HTTP servers from previous PRs. I think it'll make the code easier to read, but just a personal preference. I consider the XdsTestServer to be a top level construct, not dependent on XdsTestServerFactory. It's just that XdsTestServerFactory gives us XdsTestServer instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it's not possible to do it in Java unless we create two separate files. Having a single file is better IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason
Xds|Http|HttpProxy
is inside the factory is so that it can have aprivate
constructor. That's because there's no way to manually create the instance without having to use the factory (thehandle
will need to come from the native code). If we were about to makeXds|Http|HttpProxy
outside, the constructor would have to bepublic
. IOW, we want theFactory
to be the only way to create the instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, yeah agree, this is better then, thanks for the explanation!