Skip to content

Feat: implement spanner CDC for Bento #340

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

anicoll
Copy link
Contributor

@anicoll anicoll commented May 22, 2025

TODO: add integration tests. Added

@anicoll anicoll changed the title WIP: implement spanner CDC for Bento Feat: implement spanner CDC for Bento May 23, 2025
@jem-davies
Copy link
Collaborator

Hi @anicoll - thanks for the contribution! 🚀 - I should (or @gregfurman) get around to taking a look in a few days. Looks like there is just one lint check failing atm - is this something you could take a look at 🙏

@anicoll
Copy link
Contributor Author

anicoll commented May 25, 2025

Hi @anicoll - thanks for the contribution! 🚀 - I should (or @gregfurman) get around to taking a look in a few days. Looks like there is just one lint check failing atm - is this something you could take a look at 🙏

I have fixed the lint issue and added some extra metadata tests to the READ func. Also made those variables.

Copy link
Collaborator

@jem-davies jem-davies left a comment

Choose a reason for hiding this comment

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

Pretty much just doc changes / nits on this review, I have taken a look at the implementation and mostly looks good but want to check again regarding the use of contexts - how does this handle shutdown gracefully etc.

Thanks

@anicoll anicoll requested a review from jem-davies June 1, 2025 23:20
Copy link
Collaborator

@jem-davies jem-davies left a comment

Choose a reason for hiding this comment

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

I think that there is a problem with gracefully shutting down this component?

I set up a gcr.io/cloud-spanner-emulator/emulator similar to the integration test then manually sent SIGTERM to the bento process and got the logs in the log.txt file I have attached.

logs.txt

Also I think that the connect logic doesn't retry because I did get this once as well:

go run main.go -c ./tmp/0605_gcp_spanner_cdc_test/config.yaml
INFO Running main config from specified file       @service=bento bento_version="" path=./tmp/0605_gcp_spanner_cdc_test/config.yaml
INFO Listening for HTTP requests at: http://0.0.0.0:4195  @service=bento
ERRO Service closing due to: failed to init input <no label> path root.input: rpc error: code = FailedPrecondition desc = Schema change operation rejected because a concurrent schema change operation or read-write transaction is already in progress.  @service=bento
exit status 1

I think normally we put most of the connection logic in the Connect function of the component then the bento engine will retry to connect indefinitely rather than terminate?

There is also a small fix needed for the tests at the moment.

@anicoll
Copy link
Contributor Author

anicoll commented Jun 9, 2025

@jem-davies
I have fixed said tests (apologies) and please see attached screenshot of the handling of a sigterm now.
image

If you want me to alter the log levels to anything specific please let me know!

I tested it like so
`
func main() {
ctx := context.Background()

go func() {
	time.Sleep(15 * time.Second)
	p, err := os.FindProcess(os.Getpid())
	if err != nil {
		panic(err)
	}
	if err := p.Signal(syscall.SIGTERM); err != nil {
		panic(err)
	}
}()

service.RunCLI(ctx)

}
`

Any more feedback is welcome..

@anicoll anicoll requested a review from jem-davies June 9, 2025 03:58
@anicoll
Copy link
Contributor Author

anicoll commented Jun 10, 2025

This PR resolves #341

@anicoll
Copy link
Contributor Author

anicoll commented Jun 20, 2025

@jem-davies ping

@jem-davies
Copy link
Collaborator

jem-davies commented Jun 20, 2025

@jem-davies ping

👋 @anicoll - haven't forgotten about - just working through a few other things on the todo list then will come back to this

If it's a blocker for you - I would suggest looking at implementing as a plugin.

@jem-davies
Copy link
Collaborator

I think that the issue regarding graceful shutdown is still an issue, was going to get round to taking a proper look and seeing if I could give more advice but currently taking a look at some other things.

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.

2 participants