Skip to content
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

Refactored TransportMessage context #7303

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@uboness
Copy link
Contributor

uboness commented Aug 16, 2014

Removed CHM in favour of an OpenHashMap and synchronized accessor/mutator methods. Also, the context is now lazily inititialied (just like we do with the headers)

Refactored TransportMessage context
Removed CHM in favour of an OpenHashMap and synchronized accessor/mutator methods. Also, the context is now lazily inititialied (just like we do with the headers)
/**
* @return A safe immutable copy of the current context of this request.
*/
public synchronized ImmutableOpenMap<Object, Object> getContext() {

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 18, 2014

Member

thinking if those synchronized code blocks are needed, if you always check for null first, as context is never set to null again...

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

@spinscale I think it is needed if it is expected that another thread might be updating the context at the same time (ie. synchronization is protecting the hash map copy rather than the null check)

@spinscale

This comment has been minimized.

Copy link
Member

spinscale commented Aug 18, 2014

one minor comment (possiblgy to ignore), like the lazy initialization..

apart from that LGTM

@@ -189,6 +189,12 @@ public int hashCode() {
return EMPTY;
}

public static <KType, VType> ImmutableOpenMap<KType, VType> of(ObjectObjectMap<KType, VType> map) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

Can you add docs? (be it just to mention that the map is cloned)

This comment has been minimized.

Copy link
@uboness

uboness Sep 4, 2014

Author Contributor

sure

This comment has been minimized.

Copy link
@jpountz

jpountz Sep 4, 2014

Contributor

This makes me think that copyOf might be a better name?

This comment has been minimized.

Copy link
@uboness

uboness Sep 4, 2014

Author Contributor

I kept it aligned with guava's ImmutableMap

* @see #putInContext(Object, Object)
*/
@SuppressWarnings("unchecked")
public final synchronized <V> V getFromContext(Object key) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

I'm not too happy with the implicit unchecked cast that is happening here. IMO it should either return an Object or require the key to be parameterized (V getFromContext(Key<V>)).

This comment has been minimized.

Copy link
@uboness

uboness Sep 4, 2014

Author Contributor

not sure I share your concern, the parameterized return value is for cleaner API (so you won't need to cast on the caller side). Not sure how that relates to the key type

This comment has been minimized.

Copy link
@jpountz

jpountz Sep 4, 2014

Contributor

Agreed that it saves a cast. But since it makes the cast implicit it also prevents the compiler from helping detect bad casts so if you do something like putInContext("key", Long.valueOf(42)); String value = getFromContext("key");, you won't get any warning. But maybe I'm being too picky here, feel free to ignore this comment, I will propose an update later on if I feel that it improves things.

@kimchy

This comment has been minimized.

Copy link
Member

kimchy commented Sep 4, 2014

LGTM

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 8, 2014

this one gets stale - @uboness wanna push this?

@clintongormley clintongormley changed the title Refactored TransportMessage context Internal: Refactored TransportMessage context Sep 8, 2014

@uboness

This comment has been minimized.

Copy link
Contributor Author

uboness commented Sep 9, 2014

this was fixed as part of 5df9c04

@uboness uboness closed this Sep 9, 2014

@clintongormley clintongormley changed the title Internal: Refactored TransportMessage context Refactored TransportMessage context Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.