-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Replace Socket, ServerSocket, and HttpServer usages in tests with mocksocket versions #22287
Conversation
This is currently blocked from being merged on a release of the mocksocket project. I should get to today or tomorrow. |
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.
this PR LGTM provided MockSocket is available publicially
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 left one comment.
@@ -19,6 +19,7 @@ | |||
|
|||
package org.elasticsearch.transport.netty4; | |||
|
|||
import org.elasticsearch.MockSocket; |
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.
My only comment is whether or not this should be in another package like org.elasticsearch.mocksocket
? In general we should avoid split packages, and in a JDK 9 world, split packages are a thing that we are definitely not going to want to do. What do you think @s1monw?
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 don't follow, what do you mean by split packages? this is unrelated to this PR no? I mean we need to fix this in MockSocket? Y
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.
Split packages are packages that live in two jars. Yes, it needs to be fixed in mocksocket but this seemed a good place to raise the conversation.
4027dea
to
7872894
Compare
I made a few changes to this PR:
The mocksocket project is in MavenCentral. So once this passes an updated review, it should be good to merge in. |
…rSocket instances.
6d53178
to
3c32279
Compare
I updated this PR with a new version of mocksocket that adds a method needed for "connect" permissions. And I rebased it on current master. |
LGTM |
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.
LGTM.
This is related to #22116. In order to remove SocketPermissions from core we need to isolate some code that can be given SocketPermissions. The mocksocket jar includes wrapped versions of Socket and SocketServer. This is one step towards migrating permissions out of core.