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

Added transient header support for TransportMessage #7187

Closed

Conversation

Projects
None yet
3 participants
@uboness
Copy link
Contributor

commented Aug 7, 2014

  • Transient headers are headers that can be set on messages yet are not serialized with them.
  • Changed header accessors/mutators so header manipulation will be done directly on the request (to void NPE with transport message headers when dealing with maps that can potentially be null)
  • Added a randomVersionCompatibleWith(Version) method to the test infrastructure to help with bwc testing
@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2014

I think it's ok to add the ability to add some context to messages, that can be reused during processing, but I don't think we should reuse headers for that, this should be a separate data-structure.

@jpountz jpountz removed the review label Aug 7, 2014

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

@jpountz fair enough... we can have another construct in the request, say... Context to take care of transient data

@jpountz

View changes

src/main/java/org/elasticsearch/transport/TransportMessage.java Outdated

/**
*
*/
public abstract class TransportMessage<TM extends TransportMessage<TM>> implements Streamable {

// a transient (not serialized with the request) key/value registry
private final Map<String, Object> context = new HashMap<>();

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 7, 2014

Contributor

I think it would be more flexible if the keys were objects? (you could have composite keys, etc.)

This comment has been minimized.

Copy link
@uboness

uboness Aug 7, 2014

Author Contributor

yeah... I guess (will change it)

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 7, 2014

Contributor

Other question: does it need to be a concurrent map?

@jpountz

View changes

src/main/java/org/elasticsearch/transport/TransportMessage.java Outdated
if (message.getHeaders() != null) {
this.headers = new HashMap<>(message.getHeaders());
if (((TransportMessage<?>) message).headers != null) {
this.headers = new HashMap<>(((TransportMessage<?>) message).headers);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 7, 2014

Contributor

This change doesn't look correct given the comment above it?

This comment has been minimized.

Copy link
@uboness

uboness Aug 7, 2014

Author Contributor

yeah... context should be copied as well

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 7, 2014

Contributor

sorry I didn't see that you created a copy (I missed the "new HashMap" part)

@jpountz

View changes

src/main/java/org/elasticsearch/transport/TransportMessage.java Outdated
}
}

public Map<String, Object> context() {
return context;
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 7, 2014

Contributor

Can you add javadocs to this method? In particular I think it's important to explain the kind of use-case that this API exists for.

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 7, 2014

Contributor

(and also the fact that it will be lost on serialization, etc.)

This comment has been minimized.

Copy link
@uboness

uboness Aug 7, 2014

Author Contributor

yep

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2014

@uboness Left some comments

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

@jpountz final pass?

@jpountz

View changes

src/main/java/org/elasticsearch/transport/TransportMessage.java Outdated
}
this.context = new HashMap<>(((TransportMessage<?>) message).context);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 7, 2014

Contributor

hmm do we really want to copy the context?

This comment has been minimized.

Copy link
@uboness

uboness Aug 7, 2014

Author Contributor

yea... I think we do... it's data that is associated with the request (like the headers)

Cleaned up TransportMessage and added transient context to it
 - The context enables setting arbitrary transient data on the message (this data is not serialized with the request)
 - Changed header accessors/mutators so header manipulation will be done directly on the request (to void NPE with transport message headers when dealing with maps that can potentially be null)
@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2014

merged with 1f9bceb

@uboness uboness closed this Aug 7, 2014

@clintongormley clintongormley changed the title Added transient header support for TransportMessage Internal: Added transient header support for TransportMessage Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Added transient header support for TransportMessage Added transient header support for TransportMessage 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.