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

Cpp producer sequence id changes #763

Merged
merged 4 commits into from
Sep 18, 2017

Conversation

merlimat
Copy link
Contributor

@merlimat merlimat commented Sep 17, 2017

Motivation

C++ and Python equivalent of the Java client library changes made in #760

Last set of changes for PIP-6 implementation.

Changes in client side to expose sequence id in the Producer and MessageBuilder interfaces.

This change will enable applications to take advantage of deduplication and ensure messages can be stored only-once even after the producer application crashes or get restarted.

Modifications

  • Added few ProducerConfiguration options:
    • setProducerName() This was already used internally by replicator. Exposed in public API
    • setInitialSequenceId() Let the application specify the initial sequence id
  • In Producer interface:
    • Added Producer.getLastSequenceId() to return the last persisted sequence id for a producer
    • Producer.getProducerName() to access the auto-generated producer name
  • MessageBuilder.setSequenceId(): manually specify the sequence id of a message. This may be related to some application specific property

@merlimat merlimat added the type/feature The PR added a new feature or issue requested a new feature label Sep 17, 2017
@merlimat merlimat added this to the 1.20.0-incubating milestone Sep 17, 2017
@merlimat merlimat self-assigned this Sep 17, 2017
@merlimat merlimat force-pushed the producer-sequence-id branch from 6e92377 to 91dceed Compare September 17, 2017 07:57
Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

LGTM.. @saandrews do you also want to take a look of it.

Copy link
Contributor

@jai1 jai1 left a comment

Choose a reason for hiding this comment

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

Minor suggestions

* <p>
* The sequence id can be used for deduplication purposes and it needs to follow these rules:
* <ol>
* <li><code>sequenceId >= 0</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Assertion for this - also why not use uint64_t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added check. Keeping int64_t for consistency with initialSequence id which can be negative.

ProducerConfiguration& setSendTimeout(int sendTimeoutMs);
int getSendTimeout() const;

ProducerConfiguration& setInitialSequenceId(int64_t initialSequenceId);
Copy link
Contributor

Choose a reason for hiding this comment

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

uint64_t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be -1 (default value)

}

ProducerConfiguration& ProducerConfiguration::setInitialSequenceId(int64_t initialSequenceId) {
impl_->initialSequenceId = Optional<int64_t>::of(initialSequenceId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Disallow negative value?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed your earlier comment. Should we allow any negative in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, initial can also be -1.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to someone setting it to -10 for example. Do we have to reject any value less than -1? Ot it doesn't matter for us?

@merlimat merlimat merged commit 824922a into apache:master Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants