kt consume: stricter parsing of offsets #105
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently parseOffsets uses a regexp-based parser which is considerably more permissive than it probably should be. It will parse almost any string without error, even such "obviously wrong" specifications such as `bogus` and `::::`. We specify the grammar somewhat more formally and rewrite the parser so that it's not regexp-based, which allows us to give better error messages, is arguably more understandable and will catch user errors before we actually try to talk to Kafka. This change is a prelude to a change that is intended to add timestamp-based search to kt. By using a more precise grammar specification, that change should become easier. We also add some missing `TestParseOffsets` test cases to cover errors and some other corners of the grammar.
|
Hi @rogpeppe - what a beautiful PR, thank you very much! I updated the tiny merge conflict and will pull the changes in! Reading through the changes, the only thoughts on changes that I had were on naming - so we could maybe try to use names that match those identifiers in the grammar. Would you be interested in reviewing tiny changes like that? I understand the delay on feedback was enormous, sorry for that! I've been a full time dad for the past 2.5 years and only resumed work on my Github projects a few days ago. Let me know if you'd be interested, especially as you mention possibly more work regarding timestamps? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently parseOffsets uses a regexp-based parser which is considerably
more permissive than it probably should be. It will parse almost
any string without error, even such "obviously wrong" specifications
such as
bogusand::::.We specify the grammar somewhat more formally and rewrite
the parser so that it's not regexp-based, which allows us to give
better error messages, is arguably more understandable and will
catch user errors before we actually try to talk to Kafka.
This change is a prelude to a change that is intended to add timestamp-based
search to kt. By using a more precise grammar specification, that change
should become easier.
We also add some missing
TestParseOffsetstest cases to cover errorsand some other corners of the grammar.