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

Allows consumer retrieve the sequence id that the producer set. #4645

Merged
merged 2 commits into from
Jul 2, 2019

Conversation

codelipenghui
Copy link
Contributor

Fixes #4643

Motivation

Allows consumer retrieve the sequence id that the producer set while enable batch producing.

Modifications

Add sequence_id for SingleMessageMetadata in PulsarApi.proto

Verifying this change

Added new unit tests.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (yes)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (yes)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@codelipenghui codelipenghui self-assigned this Jul 1, 2019
@codelipenghui codelipenghui added this to the 2.4.1 milestone Jul 1, 2019
@codelipenghui
Copy link
Contributor Author

run java8 tests

1 similar comment
@codelipenghui
Copy link
Contributor Author

run java8 tests

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of comments

@@ -193,6 +195,7 @@ enum ProtocolVersion {
v13 = 13; // Schema-registry : added avro schema format for json
v14 = 14; // Add CommandAuthChallenge and CommandAuthResponse for mutual auth
// Added Key_Shared subscription
v15 = 15; // Added sequence_id for SingleMessageMetadata, retrieve the sequence id in batch message.
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to add a new version here because adding the field on the proto message will not cause any problem if the other end (broker or client) is running an older version.
In protobuf, unknown fields are automatically ignored.

We just increase the version when adding a new feature that requires a minimum version from the peers, so that we can check pre-emptively.

@@ -737,5 +737,69 @@ public void testOrderingOfKeyBasedBatchMessageContainer() throws PulsarClientExc
producer.close();
}

@Test(dataProvider = "containerBuilder", timeOut = 3000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Test(dataProvider = "containerBuilder", timeOut = 3000)
@Test(dataProvider = "containerBuilder")

Timeout is already enforced by default (and using a small timeout increases the chances of flaking when running in Jenkins).

consumer.close();
}

@Test(dataProvider = "containerBuilder", timeOut = 3000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Test(dataProvider = "containerBuilder", timeOut = 3000)
@Test(dataProvider = "containerBuilder")

@merlimat merlimat added the type/bug The PR fixed a bug or issue reported a bug label Jul 2, 2019
@codelipenghui
Copy link
Contributor Author

@merlimat I have addressed your comments, please take a look.

@codelipenghui
Copy link
Contributor Author

run java8 tests

2 similar comments
@codelipenghui
Copy link
Contributor Author

run java8 tests

@codelipenghui
Copy link
Contributor Author

run java8 tests

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@merlimat merlimat merged commit d107f67 into apache:master Jul 2, 2019
easyfan pushed a commit to easyfan/pulsar that referenced this pull request Jul 26, 2019
…he#4645)

* Allows consumer retrieve the sequence id that the producer set.

* fix comments.
jiazhai pushed a commit that referenced this pull request Aug 28, 2019
* Allows consumer retrieve the sequence id that the producer set.

* fix comments.

(cherry picked from commit d107f67)
@codelipenghui codelipenghui deleted the issue-4643 branch May 19, 2021 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per message sequence id is lost in message batch
3 participants