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

Stopping infinite goroutines #465

Open
nikhilmat opened this issue Aug 7, 2017 · 7 comments
Open

Stopping infinite goroutines #465

nikhilmat opened this issue Aug 7, 2017 · 7 comments

Comments

@nikhilmat
Copy link
Contributor

nikhilmat commented Aug 7, 2017

Hey all!

Thanks for building such an awesome library!

We're getting ready to start using this as a part of our production workflow, and we wanted to build a wrapper around the gh-ost utility to manage some configuration on our end and to integrate it into our development/deployment workflows. We were hoping to build this wrapper in Go and consume the library directly.

When building out the proof of concept, we noticed that migrator.go and a few other places are starting infinite goroutines (for example the initiateStatus) that seem to be relying on the fact that it is invoked from a CLI and that the program will exit when Migrate() is complete. The behavior we saw was even after the migration completed, statuses were still being reporting to STDOUT.

The wrapper we are building will be a long running process (http server) that will manage the invocations of the gh-ost library. Would you be willing to accept a PR that cleans up these infinite goroutines, allowing the core logic to be used outside of the single CLI run?

Happy to hear any feedback on this approach if not. Thanks again!

@nikhilmat nikhilmat changed the title Fixing leaked goroutines Stopping infinite goroutines Aug 7, 2017
@shlomi-noach
Copy link
Contributor

@nikhilmat thank you!. Indeed the tool was designed to run as standalone, but there's no reason why it should have to stay that way.
I'm happy to receive PRs to change that, please go ahead.

I'm also curious about the wrapper service; will this be a schema deployment management tool, in the likeness of shift?

@Xopherus
Copy link

I'm definitely interested in this service as well! Would love to be able to use gh-ost to manage table alterations but have the changes be versioned/deployed with a schema management tool.

@tomkrouper
Copy link

tomkrouper commented Aug 11, 2017

@Xopherus another thing we plan on looking at in the next quarter is https://github.com/skeema/skeema to see if we can use it as part of our schema management and changes.

@nikhilmat
Copy link
Contributor Author

Sorry for the late follow up on this issue! Finally got around to finishing up the changes and submitting a PR, looking forward to hearing feedback.

As far as how we will be using this wrapper service, it won't be a full blown schema management tool on it's own. We are building something like shift internally, but only for executing migrations - we wanted to continue to drive the schema changes by code checked into the applications, rather than submitting them via UI. This wrapper service however will be used to integrate into a tool that we will use to kick off, monitor, and interact with migrations that are executed by gh-ost.

The piece wrapping gh-ost we had intended to simply orchestrate multiple gh-ost migrations, contain logic/permissions for accessing our databases, support CREATE/DROP tables with a similar API, and contain the throttling/hook logic that we will share across our application's migrations.

Happy to share more about what we've built once we have the proof of concept put together!

@timvaillancourt
Copy link
Collaborator

@nikhilmat I'm cleaning up old issues. Were you able to implement any fixes for infinite goroutines?

@RainbowDashy
Copy link

@timvaillancourt I believe @nikhilmat has implemented in #479.

By the way, I fixed some missing cleanups downstream bytebase#3, would you like to have it merged on the upstream?

@timvaillancourt
Copy link
Collaborator

By the way, I fixed some missing cleanups downstream bytebase#3, would you like to have it merged on the upstream?

@RainbowDashy thanks! An upstream PR sounds useful 🙏

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

No branches or pull requests

6 participants