-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
add ClientFilter to HttpReplicationClient #1212
add ClientFilter to HttpReplicationClient #1212
Conversation
@@ -15,4 +16,5 @@ | |||
|
|||
EurekaHttpResponse<ReplicationListResponse> submitBatchUpdates(ReplicationList replicationList); | |||
|
|||
default void addReplicationClientFilter(ClientFilter clientFilter) {}; |
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.
Adding this method here would force a Jersey dependency on HttpReplicationClient. You should be able to provide your own PeerEurekaNodes implementation similar to Jersey2PeerEurekaNodes that would add filters to the JerseyReplicationClient.
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.
So no need for this change, but the one below is still valid, correct?
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.
Given the static nature of these API's I'd say yes. Ideally I'd rather have a proper factory and only allow adding filters at creation time. Not sure about the thread safety guarantees when adding a filter to an existing client. Not sure it's worth the effort to refactor that now.
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.
A new PeerEurekaNodes
implementation itself isn't enough to add filters to JerseyReplicationClient
because JerseyReplicationClient.createReplicationClient()
returns HttpReplicationClient
, that's why I went ahead and changed the interface. But I agree that it's not great to force a Jersey dependency on that client.
So maybe I can try implementing the second option proposed in the issue, which is passing in additional filters on the static creation method, or use ReplicationClientOptionalArgs
if we follow the pattern of DiscoveryClient using DiscoveryClientOptionalArgs
.
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.
JerseyReplicationClient.createReplicationClient() returns a JerseyReplicationClient and not HttpReplicationClient.
eureka/eureka-core/src/main/java/com/netflix/eureka/transport/JerseyReplicationClient.java
Line 135 in 07876a7
public static JerseyReplicationClient createReplicationClient(EurekaServerConfig config, ServerCodecs serverCodecs, String serviceUrl) { |
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.
We don't interact directly with that. We have a bean that extends PeerEurekaNodes
. I guess we'd override createPeerEurekaNode()
?
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.
RE: concurrency, we would only add a client filter before eureka server starts up during the spring lifecycle.
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.
@elandau I updated this PR removing the addReplicationClientFilter
method from the interface.
@elandau Based on your comment of "only allow adding filters at creation time", I did this quick spike on how that could look like: master...LittleBaiBai:add-additional-filters-on-creation. I haven't tested this yet. Do you think this is on the right (or better) direction? |
@LittleBaiBai I think the current implementation (this PR) is fine. I actually view the existing pattern of passing in an object of optional dependencies as poor api design. That pattern came about as a hack around Guice's poor support for optional injection. If we ever decide to refactor this code I'd prefer to deprecate all constructors and switch to the builder pattern. |
Connects to #1211