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

KAFKA-3977: defer fetch response parsing and raise exceptions to user #1245

Merged
merged 4 commits into from
Oct 11, 2017

Conversation

dpkp
Copy link
Owner

@dpkp dpkp commented Oct 8, 2017

See #1141



def test__unpack_message_set():
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still WIP?

@dpkp
Copy link
Owner Author

dpkp commented Oct 10, 2017

@tvoinarovskyi does this PR conflict with your work? do you want me to wait until your changes are ready to land or are you ok rebasing + dealing with any conflicts this may cause?

@tvoinarovskyi
Copy link
Collaborator

@dpkp no, rather I would like to merge this first. I did not find time to implement deferred parsing myself)

def has_more(self):
return self.messages and self.message_idx < len(self.messages)

class FetchResponseMetricAggregator(object):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this aggregator behave properly in case CompletedFetch is not parsed? Say we parse 2 of 3 partitions and the user uses seek() which will cause the CompletedFetch to be discarded.

@tvoinarovskyi
Copy link
Collaborator

@dpkp I will merge this now. If you feel like some changes are needed please address them as a separate PR. I would like to rebase my PR on top to make it ready for merge, I think it's ready for it.

@tvoinarovskyi tvoinarovskyi merged commit f04435c into master Oct 11, 2017
@jeffwidman jeffwidman deleted the KAFKA_3977_defer_fetch_parsing branch October 11, 2017 22:08
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