-
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
Conversation
Signed-off-by: Fredy Wijaya <[email protected]>
/retest |
/assign @abeyad |
/** An xDS test server factory. */ | ||
public class XdsTestServerFactory { | ||
/** The instance of {@link XdsTestServer}. */ | ||
public static class XdsTestServer { |
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 a private
constructor. That's because there's no way to manually create the instance without having to use the factory (the handle
will need to come from the native code). If we were about to make Xds|Http|HttpProxy
outside, the constructor would have to be public
. IOW, we want the Factory
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!
Rewrite JNI code for XdsTestServer Signed-off-by: Fredy Wijaya <[email protected]>
This PR rewrites the JNI implementation for
XdsTestServer
to follow a similar style asHttpTestServer
andHttpProxyTestServer
. This PR also eliminates the use of YAML in the test.Risk Level: low (test only)
Testing: unit test
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a