Skip to content

Correct typo for example in README #20

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

Merged
merged 2 commits into from
Jul 29, 2016
Merged

Conversation

dfdeshom
Copy link
Contributor

No description provided.

@ghost
Copy link

ghost commented Jul 21, 2016

It looks like @dfdeshom hasn't signed our Contributor License Agreement, yet.

Appreciation of efforts,

clabot

@ewencp
Copy link
Contributor

ewencp commented Jul 27, 2016

Hmm, @dfdeshom the intent with that block is that something could potentially set running to False so the loop could exit and the close() method could run. I get why you would change it to True though -- nothing ever changes it. Maybe we could instead change it so we set running = False at some point? The close() call is useful as it shows how to correctly clean up.

@dfdeshom
Copy link
Contributor Author

@ewencp sounds good. My focus was on code that I can copy and paste to a console and have it work. Something like you mentioned would work:

running = True
while running:
    msg = c.poll()
    if not msg.error():
        print('Received message: %s' % msg.value().decode('utf-8'))
    else:
        running = False
c.close()

@ewencp
Copy link
Contributor

ewencp commented Jul 29, 2016

@dfdeshom Yeah, that absolutely makes sense. I like the suggested change, want to update the PR?

Update the example to show that upon exit, the consumer must call `close()`
@dfdeshom
Copy link
Contributor Author

Done!

@ewencp
Copy link
Contributor

ewencp commented Jul 29, 2016

Awesome, LGTM. Thanks @dfdeshom! Last bit of formality before merging is the CLA. Not sure why the bot didn't include the link, but you can sign it here: clabot.confluent.io/cla

@dfdeshom
Copy link
Contributor Author

Done! CLA should be signed

@ewencp
Copy link
Contributor

ewencp commented Jul 29, 2016

@dfdeshom Excellent, thanks for the contribution!

@ewencp ewencp merged commit cb0d0af into confluentinc:master Jul 29, 2016
@edenhill
Copy link
Contributor

I'm sorry for coming in late but this change will cause the loop to exit as soon as the end is reached for any consumed partition (error is ._PARTITION_EOF).

We've discussed not emitting that event as an error but in the meantime that particular error will need to be handled explicitly in the example loop.

@ewencp
Copy link
Contributor

ewencp commented Jul 29, 2016

@edenhill Yeah, the tradeoff here is verbosity vs completeness. The goal here was just getting to something that'll actually exit. Not sure how robust/complete an example in a README necessarily needs to be.

@edenhill
Copy link
Contributor

edenhill commented Aug 4, 2016

@ewencp I agree, but this introduces behaviour that is generally undesired: exiting on the first partition EOF, and we dont want people to start off their experience with that behaviour.
I think the initial code was more clear: leaving it to the developer to decide what sets running to false since that is a highly app-specific thing and is easy to reason about.

@dfdeshom
Copy link
Contributor Author

dfdeshom commented Aug 4, 2016

I can update the example if needed.

I guess it was implicit in the previous version that users should determine what sets running to False, but that wasn't made clear.

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.

3 participants