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

DBZ-3226: Add step to verify the Debezium-server distribution #140

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

Sgitario
Copy link
Contributor

Added a new job to validate the distribution works for each of the supported sinks. For now, I only added the "redis" sink.

Fixes https://issues.redhat.com/browse/DBZ-3226

@@ -70,3 +70,45 @@ jobs:
run: ./server/mvnw clean install -f core/pom.xml -DskipTests -DskipITs -Dformat.formatter.goal=validate -Dformat.imports.goal=check -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=120
- name: Maven build Debezium Server
run: ./server/mvnw clean install -fae -f server/pom.xml -Passembly -Dformat.formatter.goal=validate -Dformat.imports.goal=check -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=120 -DskipNonCore

validate-distribution:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this job needs to be added also to the maven.yml, so I'm not sure why there are two files for building, so I prefer not duplicating it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sgitario One is executed on pull requests. That job migh need to build core not from the main branch but from a different branch (same name as this one). This is used for cases when PR depends on changes in core repo.
maven.yml is used exclusively for main/branch builds and always build from main of the core repo

@Sgitario Sgitario force-pushed the DBZ-3226 branch 2 times, most recently from d1d9ddd to b75af1b Compare December 12, 2024 13:23
@@ -70,3 +70,53 @@ jobs:
run: ./server/mvnw clean install -f core/pom.xml -DskipTests -DskipITs -Dformat.formatter.goal=validate -Dformat.imports.goal=check -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=120
- name: Maven build Debezium Server
run: ./server/mvnw clean install -fae -f server/pom.xml -Passembly -Dformat.formatter.goal=validate -Dformat.imports.goal=check -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false -Dmaven.wagon.httpconnectionManager.ttlSeconds=120 -DskipNonCore

validate-distribution:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if a different job is needed as it executes yet another build. Can't the test be part of `build step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to have a different job to use the strategy.matrix, so we can execute the same job to cover other different sinks / configurations with little effort.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the built artifact can be shared between jobs?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Mario, if we can reuse the build across jobs since there is this build dependency defined, I think that's reasonable to reduce the overall execution time on CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mfvitale @Naros PR updated to reuse the distribution from the Build job.
I confirmed that the validation job now takes a few seconds when before it used to take some minutes.

with:
distribution: 'temurin'
java-version: 21
- name: Checkout Debezium Server repository
Copy link
Member

Choose a reason for hiding this comment

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

I think this is no more required right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to check out the sink specific test resources which is used to verify the server

Copy link
Contributor

@jpechane jpechane left a comment

Choose a reason for hiding this comment

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

@Sgitario I think we've converged on good solution!
I left a comment WRT postgres exmaple version.

One mor eproposal for an imporvement - maybe a follow-up Jira. Would it be possible to connect to JMX as part of test evaluation and check metrics to make sure that expected number of messages was captured - effectivelly asserting https://debezium.io/documentation/reference/3.0/connectors/postgresql.html#connectors-snaps-metric-totalnumberofeventsseen_postgresql

@Sgitario
Copy link
Contributor Author

One mor eproposal for an imporvement - maybe a follow-up Jira. Would it be possible to connect to JMX as part of test evaluation and check metrics to make sure that expected number of messages was captured - effectivelly asserting https://debezium.io/documentation/reference/3.0/connectors/postgresql.html#connectors-snaps-metric-totalnumberofeventsseen_postgresql

This sounds good +1. I've just created https://issues.redhat.com/browse/DBZ-8518 to add this. Also, as a follow-up task and when this functionality is more consolidated, we should also add the rest of the sinks to it.

@jpechane I've just squashed all the commits.

@jpechane jpechane merged commit 4c214e2 into debezium:main Dec 18, 2024
3 checks passed
@jpechane
Copy link
Contributor

@Sgitario Applied, thanks!

@Sgitario Sgitario deleted the DBZ-3226 branch December 18, 2024 07:19
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.

4 participants