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

chore: add better test coverage for sortedSetGetRank #488

Merged
merged 1 commit into from
May 11, 2023

Conversation

pgautier404
Copy link
Contributor

An issue with grpc-js is causing the SortedSetGetRank operation to return undefined as the rank value for the topmost (0-th ranked according to sort order) item in the set. All other rank values are fine, and the same behavior is not observed with grpc-web and the web sdk.

This commit introduces a test to demonstrate the bug as well as a temporary workaround that detects the combination of a hit cache result and an undefined rank value and changes the rank to zero. This will prevent the top-ranked item in a sorted set from being mishandled as a cache miss. This change is intended to be removed when a root cause and remediation are found.

@krispraws
Copy link
Contributor

I think upgrading protoc-gen-ts to 0.8.5 in momentohq/client-protos will likely fix this since it changes the accessors in the generated ts files to return default values. But it is a breaking change and I don't have enough context to know if it will affect anything else in our SDK.
protoc-gen-ts v 0.6.0 (current version) generates _SortedSetGetRankResponse::_RankResponsePart::get_rank() as

            get rank() {
                return pb_1.Message.getField(this, 2) as number;
            }

protoc-gen-ts v 0.8.5 generates

            get rank() {
                return pb_1.Message.getFieldWithDefault(this, 2, 0) as number;
            }

cprice404 added a commit that referenced this pull request May 10, 2023
This commit updates us to the latest versions of the generated
client-protos, which includes a newer version of protogen and
grpc-js.  This was inspired by this comment related to a bug
that we recently discovered:

#488 (comment)

Haven't run the new test case yet to confirm whether this addresses
it, but upgrading to the latest should be for the best regardless.
@cprice404
Copy link
Contributor

holding off on this until #494 is merged; then I want to run the new test without the workaround, to see if the thing that @krispraws found will solve it. If not, then I think this hack will be the right way to go.

@cprice404 cprice404 changed the title fix: work around bug that returns rank value 0 as undefined chore: add better test coverage for sortedSetGetRank May 10, 2023
@cprice404 cprice404 changed the base branch from main to latest-protogen May 10, 2023 18:15
@cprice404
Copy link
Contributor

I rebased this on to my other branch where I was updating the protogen, and changed the name accordingly. When running the tests locally, the fix from protogen made the new test pass so that is now the only change in this PR.

Copy link
Contributor

@krispraws krispraws left a comment

Choose a reason for hiding this comment

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

:shipit:

Base automatically changed from latest-protogen to main May 11, 2023 16:53
@cprice404 cprice404 merged commit 0485527 into main May 11, 2023
8 checks passed
@cprice404 cprice404 deleted the work-around-undef-rank branch May 11, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants