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
wants to merge 1 commit into from
Closed

Added transient header support for TransportMessage #7187

wants to merge 1 commit into from

Conversation

uboness
Copy link
Contributor

@uboness uboness 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
Copy link
Contributor

jpountz 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
Copy link
Contributor Author

uboness commented Aug 7, 2014

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


/**
*
*/
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<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah... I guess (will change it)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jpountz
Copy link
Contributor

jpountz commented Aug 7, 2014

@uboness Left some comments

@uboness
Copy link
Contributor Author

uboness commented Aug 7, 2014

@jpountz final pass?

}
this.context = new HashMap<>(((TransportMessage<?>) message).context);
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm do we really want to copy the context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

 - 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
Copy link
Contributor Author

uboness 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
dnhatn added a commit that referenced this pull request Jul 7, 2020
If the recovery source is on an old node (before 7.2), then the recovery
target won't have the safe commit after phase1 because the recovery
source does not send the global checkpoint in the clean_files step. And
if the recovery fails and retries, then the recovery stage won't
transition properly. If a sync_id is used in peer recovery, then the
clean_files step won't be executed to move the stage to TRANSLOG.

Relates ##7187
Closes #57708
dnhatn added a commit that referenced this pull request Jul 7, 2020
If the recovery source is on an old node (before 7.2), then the recovery
target won't have the safe commit after phase1 because the recovery
source does not send the global checkpoint in the clean_files step. And
if the recovery fails and retries, then the recovery stage won't
transition properly. If a sync_id is used in peer recovery, then the
clean_files step won't be executed to move the stage to TRANSLOG.

Relates ##7187
Closes #57708
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.

None yet

3 participants