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

fix: allow setting auto.offset.reset=latest on /query-stream endpoint #5455

Merged
merged 2 commits into from
May 22, 2020

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented May 22, 2020

Description

Not only does the /query-stream endpoint default to auto.offset.reset=earliest, it currently doesn't allow the user to specify auto.offset.reset=latest since the endpoint hardcodes putting auto.offset.reset=earliest into the properties map, which overrides any user specification for the property value.

This PR fixes the issue in a really hacky way. The solution is brittle since it requires checking for all possible aliases of the auto.offset.reset property. Ideally we'd evaluate the properties first and then apply the override but that would involve changes to the engine.

Testing done

Added an integration test.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@vcrfxia vcrfxia requested a review from a team as a code owner May 22, 2020 00:37
@vcrfxia vcrfxia requested a review from purplefox May 22, 2020 00:37
Copy link
Contributor

@purplefox purplefox left a comment

Choose a reason for hiding this comment

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

LGTM

@vcrfxia vcrfxia merged commit d91c016 into confluentinc:master May 22, 2020
@vcrfxia vcrfxia deleted the query-stream-auto-offset-reset branch May 22, 2020 13:19
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.

2 participants