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

Bugfix for: Ensuring that JerseyRequest scoped ClientConfig gets prop… #1137

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@velocipedist
Contributor

velocipedist commented Jun 26, 2015

…agated to HttpUriRequest #939

The change makes the config changes introduced via Invocation.Builder#property(propertyName, propertyValue) effective. We want to use this pattern because it is truely request creation scoped. In contrast the current implementation is only effective if config changes are being introduced via WebTarget#property(propertyName, propertyValue) which unfortunatly creates a new copy of copy of org.glassfish.jersey.client.ClientRuntime which effectively makes it innefective for implementing dynamic configuration (we saw OutOfMemmory errors due to that).

Original change can be founde here: #939

Bugfix for: Ensuring that JerseyRequest scoped ClientConfig gets prop…
…agated to HttpUriRequest #939

The change makes the config changes introduced via `Invocation.Builder#property(propertyName, propertyValue)` effective. We want to use this pattern because it is truely request creation scoped. In contrast the current implementation is only effective if config changes are being introduced via `WebTarget#property(propertyName, propertyValue)` which unfortunatly creates a new copy of copy of `org.glassfish.jersey.client.ClientRuntime` which effectively makes it innefective for implementing dynamic configuration (we saw OutOfMemmory errors due to that).

Original change can be founde here: #939 (comment)

@jplock jplock added this to the 0.9.0 milestone Jun 26, 2015

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Jun 26, 2015

Member

This LGTM, any feedback @arteam ?

Member

jplock commented Jun 26, 2015

This LGTM, any feedback @arteam ?

@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Jun 26, 2015

Contributor

@jplock I noticed you've added it to the 0.9.0 milestone. Does this mean there will be no 0.8.2?

Contributor

velocipedist commented Jun 26, 2015

@jplock I noticed you've added it to the 0.9.0 milestone. Does this mean there will be no 0.8.2?

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Jun 26, 2015

Member

@velocipedist correct, we already cut 0.9.0-rc1, so ideally this would make it into rc2

Member

jplock commented Jun 26, 2015

@velocipedist correct, we already cut 0.9.0-rc1, so ideally this would make it into rc2

@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Jun 26, 2015

Contributor

A bit OT, but will ask anyway.

@jplock what are the timelines for this? I'm asking because we found a bug with the metrics library version used by 0.8.1 where request durations are not reported correctly for async resource usage. We were hoping to patch this (unless already done) before the next release of dw.

Contributor

velocipedist commented Jun 26, 2015

A bit OT, but will ask anyway.

@jplock what are the timelines for this? I'm asking because we found a bug with the metrics library version used by 0.8.1 where request durations are not reported correctly for async resource usage. We were hoping to patch this (unless already done) before the next release of dw.

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Jun 26, 2015

Member

Unfortunately the metrics releases are a bit out of our hands. @ryantenney has been working on a roadmap for v4.x but I don't have any visibility into its release schedule. It may be best to work around it manually for now if you have a workaround. We're hoping to get Dropwizard 0.9.0 out in July. /cc @carlo-rtr

Member

jplock commented Jun 26, 2015

Unfortunately the metrics releases are a bit out of our hands. @ryantenney has been working on a roadmap for v4.x but I don't have any visibility into its release schedule. It may be best to work around it manually for now if you have a workaround. We're hoping to get Dropwizard 0.9.0 out in July. /cc @carlo-rtr

@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Jun 26, 2015

Contributor

FYI: @chrisgray (see the comment around release schedules and metrics)

Contributor

velocipedist commented Jun 26, 2015

FYI: @chrisgray (see the comment around release schedules and metrics)

@arteam arteam closed this in 5a0dde2 Jun 26, 2015

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Jun 26, 2015

Member

This also looks good to me. I've applied the patch to master with some commit message polishing.

Member

arteam commented Jun 26, 2015

This also looks good to me. I've applied the patch to master with some commit message polishing.

@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Jun 26, 2015

Member

Metrics 4 isn't going to happen anytime soon. I wanted to get a 3.2 release out, with some fixes/small features like this. We could probably make that happen before the Dropwizard 0.9 release.

Member

ryantenney commented Jun 26, 2015

Metrics 4 isn't going to happen anytime soon. I wanted to get a 3.2 release out, with some fixes/small features like this. We could probably make that happen before the Dropwizard 0.9 release.

@arteam arteam referenced this pull request Jun 29, 2015

Closed

Upgrade to Jersey 2.18 #1138

jplock added a commit that referenced this pull request Jul 1, 2015

Ensuring that request-scoped config gets propagated to HttpUriRequest
The change makes the config changes introduced via
`Invocation.Builder#property(propertyName, propertyValue)` effective.
We want to use this pattern because it is truely request creation scoped.
In contrast the current implementation is only effective if config changes
are being introduced via `WebTarget#property(propertyName, propertyValue)`
which unfortunatly creates a new copy of copy of
`org.glassfish.jersey.client.ClientRuntime` which effectively makes it
innefective for implementing dynamic configuration (we saw OutOfMemory
errors due to that).

Original change can be founde here: #939

Closes #1137

@jplock jplock modified the milestones: 0.8.2, 0.9.0 Jul 1, 2015

chrisgray pushed a commit to yammer/tenacity that referenced this pull request Sep 2, 2015

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