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

Add kafka_get_watermarks() function #34

Merged
merged 2 commits into from
May 14, 2019
Merged

Conversation

zilder
Copy link
Contributor

@zilder zilder commented Mar 29, 2019

This is a PR for the issue #33. Along with adding new function I also did a bit of refactoring mainly related to KafkaFdwGetConnection() in order to make it more versatile.

Copy link
Member

@rapimo rapimo left a comment

Choose a reason for hiding this comment

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

looks good to me.
However we kafka_fdw--0.0.1.sql should bot be touched as we introduce a new version here.
Review and adjustments would be easier if you develop on a branch of the same repo, this way I can't add simple commits and need to hand back changes to you

@zilder
Copy link
Contributor Author

zilder commented Apr 12, 2019

However we kafka_fdw--0.0.1.sql should bot be touched as we introduce a new version here.

I didn't touch kafka_fdw--0.0.1.sql, I just added it to the repo because before it was generated by make in the build time.

Review and adjustments would be easier if you develop on a branch of the same repo, this way I can't add simple commits and need to hand back changes to you

I pushed watermarks_func to the main repo. Should I create a new pull request?

@zilder
Copy link
Contributor Author

zilder commented Apr 12, 2019

Oh, now I see that kafka_get_watermarks() was added to kafka_fdw--0.0.1.sql by accident..

@zilder
Copy link
Contributor Author

zilder commented Apr 12, 2019

..fixed this and also merged with master

@rapimo
Copy link
Member

rapimo commented Apr 18, 2019

mmmh now it's not mergable anymore, can you rebase onto master?

@rapimo
Copy link
Member

rapimo commented May 1, 2019

@zilder can finalize this?

@zilder
Copy link
Contributor Author

zilder commented May 1, 2019

yes, actually i didn’t see any conflicts

@rapimo
Copy link
Member

rapimo commented May 6, 2019

well github says so:

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

…so some

refactoring (make KafkaGetConnection() independent from execution state;
extract partition list fetching code to a separate function)
@rapimo rapimo merged commit d550edc into adjust:master May 14, 2019
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

2 participants