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.

Sign up for GitHub

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

Jump to bottom

refactor(stream): change the encoding of stream consumer group #2384

Merged
merged 14 commits into from
Jul 17, 2024
Merged

refactor(stream): change the encoding of stream consumer group #2384

merged 14 commits into from
Jul 17, 2024

Conversation

Yangsx-1
Copy link
Contributor

The encoding way of stream consumer group is not efficient, this PR changes the encoding according to this discussion.

@PragmaTwice PragmaTwice mentioned this pull request Jul 1, 2024
Merged
PragmaTwice
PragmaTwice previously approved these changes Jul 1, 2024
View reviewed changes
@PragmaTwice
Copy link
Member

Should StreamEntryID::Maximum be {UINT64_MAX - 1, ..} to avoid the -1 case here?

@Yangsx-1
Copy link
Contributor Author

Yangsx-1 commented Jul 7, 2024

Can we merge this PR?

@git-hulk
Copy link
Member

@Yangsx-1 You can merge PR once the CI passed

git-hulk
git-hulk previously approved these changes Jul 10, 2024
View reviewed changes
@PragmaTwice
Copy link
Member

PragmaTwice commented Jul 10, 2024 •
edited
Loading

There is one issue that needs to be considered: after the modification, the case where EID MS is -1 needs to be excluded when traversing (iterating) all sub-keys (EID MS|EID SEQ).

As I mentioned here.

@Yangsx-1 Yangsx-1 dismissed stale reviews from PragmaTwice and git-hulk via 36e4483 July 11, 2024 15:18
@Yangsx-1 Yangsx-1 force-pushed the change-stream-encoding branch from 5f359e9 to 36e4483 Compare July 11, 2024 15:18
@Yangsx-1
Copy link
Contributor Author

There is one issue that needs to be considered: after the modification, the case where EID MS is -1 needs to be excluded when traversing (iterating) all sub-keys (EID MS|EID SEQ).

As I mentioned here.

It's been changed now.

git-hulk
git-hulk previously approved these changes Jul 11, 2024
View reviewed changes
@git-hulk git-hulk requested a review from PragmaTwice July 11, 2024 15:30
PragmaTwice
PragmaTwice previously approved these changes Jul 13, 2024
View reviewed changes
PragmaTwice
PragmaTwice reviewed Jul 13, 2024
View reviewed changes
tests/gocase/unit/type/stream/stream_test.go
@@ -306,7 +306,7 @@ var streamTests = func(t *testing.T, enabledRESP3 string) {
require.Len(t, rdb.XRange(ctx, "vipstream", "(1-0", "(42-42").Val(), 1)
require.ErrorContains(t, rdb.XRange(ctx, "vipstream", "(-", "+").Err(), "ERR")
require.ErrorContains(t, rdb.XRange(ctx, "vipstream", "-", "(+").Err(), "ERR")
require.ErrorContains(t, rdb.XRange(ctx, "vipstream", "(18446744073709551615-18446744073709551615", "+").Err(), "ERR")
require.ErrorContains(t, rdb.XRange(ctx, "vipstream", "(18446744073709551614-18446744073709551615", "+").Err(), "ERR")
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if it's 18446744073709551615 instead of 18446744073709551614 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There would be a nil answer rather than an error.

@PragmaTwice
Copy link
Member

Hi @torwig, could you please review this PR when you have some time?

@torwig
Copy link
Contributor

torwig commented Jul 15, 2024

@PragmaTwice Sure. I'll review it today.

@torwig
Copy link
Contributor

torwig commented Jul 15, 2024

@Yangsx-1 @PragmaTwice Since we have UINT64_MAX as a delimiter, should it be an invalid seq of the stream entry ID?
Should we consider changing code here, here and here?

@Yangsx-1
Copy link
Contributor Author

@Yangsx-1 @PragmaTwice Since we have UINT64_MAX as a delimiter, should it be an invalid seq of the stream entry ID? Should we consider changing code here, here and here?

Yes, i'll change these code later.

@Yangsx-1 Yangsx-1 force-pushed the change-stream-encoding branch from dc3d2c5 to 9ca0a81 Compare July 16, 2024 16:38
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 16, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@PragmaTwice PragmaTwice merged commit a2a3826 into apache:unstable Jul 17, 2024
29 of 30 checks passed
@Yangsx-1 Yangsx-1 deleted the change-stream-encoding branch July 17, 2024 10:40
@PokIsemaine PokIsemaine mentioned this pull request Aug 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PragmaTwice PragmaTwice PragmaTwice approved these changes

@torwig torwig torwig approved these changes

@git-hulk git-hulk git-hulk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Yangsx-1 @PragmaTwice @git-hulk @torwig