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

Asynchronous sendStatus(), pass WAL position to handler, fix plugin args etc #2

Merged
merged 18 commits into from
Dec 30, 2018

Conversation

blind-oracle
Copy link
Contributor

@blind-oracle blind-oracle commented Jul 4, 2018

Thanks for a very good work on pgoutput.
Some changes that I did for our project that may be useful to somebody and maybe worth integrating to the base project.

  • Send status to the PostgreSQL asynchronously - useful when the handler takes a long time to process an event
  • Pass WAL position to handler (in case it needs to track it)
  • Add the ability to specify starting WAL position and skip the log events from past if PostgreSQL sends them for some reason
  • Some bug fixes like ticker leakage
  • Don't remove replication slot as the WAL position is tracked there
  • Remove vendor folder

@blind-oracle blind-oracle changed the title Asynchronous sendStatus(), pass WAL position to handler, fix plugin args Asynchronous sendStatus(), pass WAL position to handler, fix plugin args etc Jul 4, 2018
@jmealo
Copy link

jmealo commented Oct 22, 2018

@kyleconroy @blind-oracle great collaboration! any chance we can get this merged upstream? Thank you for your hard work!

@kyleconroy
Copy link
Owner

@jmealo sure! I honestly had forgotten about this pull request 😬

@blind-oracle do you have time to make some changes? I know this code is from a while back.

Copy link
Owner

@kyleconroy kyleconroy left a comment

Choose a reason for hiding this comment

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

Overall these changes look great. Can you walk me through why you've deleted the vendored code?

@@ -5,8 +5,8 @@ import (
"fmt"
"log"

"github.com/blind-oracle/pgoutput"
Copy link
Owner

Choose a reason for hiding this comment

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

I know this is useful for testing, but this needs to be reverted before merging.

@@ -50,8 +49,8 @@ func main() {
return nil
}

sub := pgoutput.NewSubscription("sub1", "pub1")
if err := sub.Start(ctx, conn, handler); err != nil {
sub := pgoutput.NewSubscription(conn, "sub1", "pub1", 1048576)
Copy link
Owner

Choose a reason for hiding this comment

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

I know this is just an example, but can we reset the offset value here to zero?

@blind-oracle
Copy link
Contributor Author

Sorry, I also forgot about it and kept committing our project-specific stuff to my fork that ended up here :)
I'll take time to clean it up in a few days and fix @kyleconroy 's notes.

Regarding vendor folder - I had conflicts between different version of libraries (pgoutput's vs our project's) if I remember correctly. Some points against committing vendor to repo also here https://github.com/golang/dep/blob/master/docs/FAQ.md#my-dependers-dont-use-dep-yet-what-should-i-do

And if you take a look at most other popular libraries - they don't commit vendor folder to repo. Maybe we should just use dep's Gopkg.toml/.lock instead if we need to pin specific versions.

@kyleconroy
Copy link
Owner

Makes sense about the vendor directory, as this is a library. Instead of committing a Gopkg.* file, I think I'll just tag a 0.1.0 release and add a go.mod file.

@blind-oracle
Copy link
Contributor Author

Ok I've done some modifications & fixes, it should be Ok now. But since the API was changed probably we want to retain current version under some tag.

@jmealo
Copy link

jmealo commented Oct 23, 2018

Thanks for looking into this!

@blind-oracle
Copy link
Contributor Author

Any chance this would be merged?

@kyleconroy kyleconroy merged commit 6f49f4f into kyleconroy:master Dec 30, 2018
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