Skip to content

DBZ-8911 Add configuration to skip heartbeat messages in Redis Stream consumer#173

Merged
jpechane merged 1 commit intodebezium:mainfrom
spicy-sauce:DBZ-8911
Apr 14, 2025
Merged

DBZ-8911 Add configuration to skip heartbeat messages in Redis Stream consumer#173
jpechane merged 1 commit intodebezium:mainfrom
spicy-sauce:DBZ-8911

Conversation

@spicy-sauce
Copy link
Copy Markdown
Contributor

@spicy-sauce spicy-sauce commented Apr 10, 2025

No description provided.

@spicy-sauce spicy-sauce changed the title DBZ-8911 Add configuration to skip heartbeat messages in Redis Stream… DBZ-8911 Add configuration to skip heartbeat messages in Redis Stream consumer Apr 10, 2025
@spicy-sauce spicy-sauce force-pushed the DBZ-8911 branch 2 times, most recently from ecdd03c to 3de60c9 Compare April 10, 2025 14:23
Copy link
Copy Markdown
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.

@spicy-sauce LGTM, thanks! I just left one comment wrt variable naming and request for slight change in tests. When addressed this is good to go.

Map<String, Object> sourceConfig = getConfigSubset(mpConfig, "debezium.source.");

// Get Redis sink configuration
Configuration configuration = Configuration.from(getConfigSubset(mpConfig, ""));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Configuration configuration = Configuration.from(getConfigSubset(mpConfig, ""));
Configuration sinkConfig = Configuration.from(getConfigSubset(mpConfig, ""));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think renaming to sinkConfig would be accurate. The variable contains all configuration properties, not just sink-specific ones. The name configuration is actually more appropriate here since it represents the complete set of properties, IMO.
Renaming it to sinkConfig would be misleading because it implies the configuration only contains sink-related properties, which isn't the case.

@jpechane WDYT?


// Wait for some time to allow heartbeats to be generated
Testing.print("Waiting for heartbeats to be generated...");
Thread.sleep(3000);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please rewrite the test to not use Thread.sleep() but Awaitility that would wait (with timeout) till something is in heartbeat stream?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.


// Wait for some time to allow heartbeats to be generated
Testing.print("Waiting for heartbeats to be generated...");
Thread.sleep(3000);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See above, here you should probably wait for 2 in redis_test stream.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

@spicy-sauce spicy-sauce force-pushed the DBZ-8911 branch 2 times, most recently from 2312869 to a3570b8 Compare April 12, 2025 11:33
@spicy-sauce
Copy link
Copy Markdown
Contributor Author

spicy-sauce commented Apr 12, 2025

I see the CI fails on something unrelated to this PR - DebeziumServerPostgresIT - @jpechane @Naros, is it a flaky test?
I just applied @jpechane's comments on the new tests introduced in this PR, so obviously it has nothing to do with that. Please, advise.

@Naros
Copy link
Copy Markdown
Member

Naros commented Apr 13, 2025

It is a flaky test @spicy-sauce, fixed in #174

@spicy-sauce
Copy link
Copy Markdown
Contributor Author

It is a flaky test @spicy-sauce, fixed in #174

Thanks, merged main into this branch.

@jpechane
Copy link
Copy Markdown
Contributor

@spicy-sauce Could you please remove the merge commit and use just plain rebase so we keep linear history? Thanks

@spicy-sauce
Copy link
Copy Markdown
Contributor Author

@spicy-sauce Could you please remove the merge commit and use just plain rebase so we keep linear history? Thanks

Done.

@jpechane jpechane merged commit f77cec3 into debezium:main Apr 14, 2025
3 checks passed
@jpechane
Copy link
Copy Markdown
Contributor

@spicy-sauce Applied, thanks!
@Naros Thanks for the flaky fix!

@spicy-sauce spicy-sauce deleted the DBZ-8911 branch April 14, 2025 12:02
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