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

Transfer command #30

Merged
merged 101 commits into from Jul 22, 2019
Merged

Transfer command #30

merged 101 commits into from Jul 22, 2019

Conversation

Bibob7
Copy link
Contributor

@Bibob7 Bibob7 commented Jul 8, 2019

No description provided.

@Bibob7
Copy link
Contributor Author

Bibob7 commented Jul 8, 2019

Still todo:

  • Write tests for new logic.

Copy link
Contributor

@hfjn hfjn left a comment

Choose a reason for hiding this comment

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

Please also run black with --line-length 119. Would be cool if you could add that to travis as well.

esque/cli/commands.py Outdated Show resolved Hide resolved
esque/clients.py Outdated Show resolved Hide resolved
esque/clients.py Outdated Show resolved Hide resolved
esque/clients.py Outdated Show resolved Hide resolved
esque/clients.py Outdated Show resolved Hide resolved
esque/message.py Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
esque/cli/commands.py Outdated Show resolved Hide resolved
esque/clients.py Outdated Show resolved Hide resolved
esque/clients.py Outdated Show resolved Hide resolved
esque/message.py Outdated Show resolved Hide resolved
esque/message.py Outdated Show resolved Hide resolved
esque/message.py Show resolved Hide resolved
esque/schemaregistry.py Outdated Show resolved Hide resolved
@Bibob7 Bibob7 requested a review from hfjn July 19, 2019 21:26
Copy link
Contributor

@hfjn hfjn left a comment

Choose a reason for hiding this comment

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

One minor comment. Otherwise approved.
Nice work and great code. 👍

number_consumed_messages = _consume_to_file(working_dir, topic, group_id, from_context, numbers, avro, last)

if number_consumed_messages == 0:
click.echo(click.style("Execution stopped, because no messages consumed.", fg="red"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would state the reason for stopping the execution. No consumption is the effect. This case is probably getting hit when the topic is empty or the starting offset was chosen to high, right?

Copy link
Contributor Author

@Bibob7 Bibob7 Jul 22, 2019

Choose a reason for hiding this comment

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

How would you do this? Maybe checking the high_watermarks for all topics as indicator for an empty topic? But with log retention it could also be empty with a high_watermark greater 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean in that much detail.
The user can still find out what exactly the problem was by him/herself. You could just name the possible reasons in the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sure

@Bibob7 Bibob7 merged commit bc2b44a into master Jul 22, 2019
@swenzel swenzel deleted the transfer-command branch April 7, 2020 11:55
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