Skip to content

Conversation

@theghost5800
Copy link
Contributor

This feature allows to include custom http headers in every http
requests using map

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 6, 2019

CLA Check
The committers are authorized under a signed CLA.

@twoseat
Copy link
Contributor

twoseat commented Nov 6, 2019

Can you explain the business case for this?

@theghost5800
Copy link
Contributor Author

Business case for this is to allow users to add some custom http header which can be used for web requests analysis tools (Dynatrace). For example you have multiple running processes in your application and every process creates it own instance of CloudFoundryClient and executes web requests to the controller. You can set correlation id which can be used to identify which process sends request and use it as filter to generate statistics.

@theghost5800
Copy link
Contributor Author

Hello @twoseat ,
This implementation actually does not work for our case. We reuse ConnectionContext for every client which we create, in this case we can't set different correlation id for every client. We reuse instance of ConnectionContext because every new instance creates new thread pool and this lead our application to threads exhaustion and OutOfMemory. First option to fix this issue is to move Map<String, String> requestTags in _ReactorCloudFoundryClient and the second option is to expose as public method getThreadPool with Value.Default annotation, in this case we can create manually thread pool and pass to ConnectionContext as parameter. With first option it will be needed to refactor all constructors which extends _ReactorCloudFoundryClient classs, with the second option it will be necessary only to add public access modifer and change annotation of getThreadPool method in _DefaultConnectionContext class. What do you think?

@nebhale
Copy link
Contributor

nebhale commented Nov 8, 2019

@theghost5800 There is a non-intuitive way to reuse the expensive resources in a DefaultConnectionContext. While you can't specifically set the connection and thread pools that are used, those are generated on-demand when you haven't specified the HttpClient. Therefore it's possible to create a DefaultConnectionContext and then reuse the created elements in another DefaultConnectionContext:

ConnectionContext cc1 = DefaultConnectionContext.builder()
    .apiHost("test-host")
    .build();

ConnectionContext cc2 = DefaultConnectionContext.builder()
    .httpClient(cc1.getHttpClient())
    .build();

That being said, you shouldn't do this. Conceptually, the design of the Java Client is much like the design of Hibernate; the ConnectionContext (SessionFactory) is an expensive to create singleton that is shared between all operations that require it. A *Client (Session) is a lightweight non-shared item that can be created and destroyed more or less for free. Therefore, while technically you can do what you want with a DefaultConnectionContext, conceptually it's the wrong place to do it.

I believe that the proper design is to add, to all *Clients, a headers(Consumer<HttpHeaders>) (might need to hide the Netty type though). That is called on every request, way down in AbstractReactorOperations or lower, allowing arbitrary header manipulation.

This feature allows to include custom http headers in every http
requests using map
@theghost5800
Copy link
Contributor Author

I did some refactoring and move out custom http headers from ConnectionContext into client

@theghost5800
Copy link
Contributor Author

Any updates about my PR?

@nebhale nebhale self-requested a review January 10, 2020 16:56
@nebhale nebhale self-assigned this Jan 10, 2020
@nebhale nebhale added this to the 3.21.0.RELEASE milestone Jan 10, 2020
nebhale pushed a commit that referenced this pull request Jan 10, 2020
Previously, there was no way to add custom HTTP headers to requests made by
the client.  This change adds configuration options to include a custom header
in all the requests made by a client.

[#1015]

Signed-off-by: Ben Hale <[email protected]>
@nebhale nebhale closed this in 5f3258d Jan 10, 2020
@nebhale
Copy link
Contributor

nebhale commented Jan 10, 2020

@theghost5800 Please grab the latest snapshots and give them a work out. If your use-case is satisfied, we'll cut a release.

@theghost5800
Copy link
Contributor Author

@nebhale Thank you for meging this feature in your repo. I will test it and write you back if it everything okay to release it.

@twoseat
Copy link
Contributor

twoseat commented Feb 13, 2020

Hi @theghost5800 - I'm prepping a release for tomorrow, so if you have any objection to this fix please let me know now!

@theghost5800
Copy link
Contributor Author

Hi @twoseat , Everythings looks fine. Thank you for including this feature in your project.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants