Skip to content
This repository was archived by the owner on Jul 27, 2023. It is now read-only.

Conversation

@rogoza-igor
Copy link

Hi!
We overloaded ClientEventCallback interface methods to be able to path some additional information (path and responseCode) to implementation. This information can be useful for example for monitoring.

/**
* @deprecated use {@link #onHttpRequestSuccess(String, String, String, String, int)} instead.
*/
@Deprecated(forRemoval = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is ok to use Java 9 style @deprecated in this project?

Copy link
Author

Choose a reason for hiding this comment

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

Deleted forRemoval attribute

public void httpRequestSuccess(Request request, Response<?> response) {
EVENT_EXECUTOR.submit(() -> {
callback.onHttpRequestSuccess(clientName, request.method(), request.url().query());
callback.onHttpRequestSuccess(clientName, request.method(), request.url().encodedPath(), request.url().query(), response.code());
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, is it good to have both callbacks executed?

Maybe it would be better to call the old deprecated method in new default one:

default void onHttpRequestSuccess(String clientName, String method, String path, String queryString, int responseCode) {
        // Kept for compatibility at the moment
        onHttpRequestSuccess(clientName, method, queryString);        
}

Then replacing calls here would still keep the backwards compatibility?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's good idea, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants