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

KAFKA-431: Use Time.SYSTEM for initializing system time #171

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

Calvinnix
Copy link
Collaborator

@Calvinnix Calvinnix commented Dec 6, 2024

Kafka removed support for the new SystemTime() instantiation as part of version 3.9

Specifically as part of this PR.

This change should be safe as far as backwards compatibility goes because that Time.SYSTEM singleton interface has existed for a long time.

@Calvinnix Calvinnix requested a review from arahmanan December 6, 2024 15:09
Copy link
Collaborator

@arahmanan arahmanan left a comment

Choose a reason for hiding this comment

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

LGTM! What do you think about filing a ticket to test against different versions of Kafka?

@@ -155,7 +154,7 @@ final class StartedMongoSourceTask implements AutoCloseable {
assertTrue(sourceConfig.getStartupConfig().startupMode() == COPY_EXISTING);
}
isCopying = shouldCopyData;
time = new SystemTime();
time = Time.SYSTEM;
Copy link
Collaborator

Choose a reason for hiding this comment

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

good find!

@Calvinnix
Copy link
Collaborator Author

LGTM! What do you think about filing a ticket to test against different versions of Kafka?

Would that do something like automatically download new versions as well? I think that's a good idea

@arahmanan
Copy link
Collaborator

LGTM! What do you think about filing a ticket to test against different versions of Kafka?

Would that do something like automatically download new versions as well? I think that's a good idea

The first iteration doesn't necessarily have to test new versions automatically. I'm not against it if it's simple enough though

@Calvinnix
Copy link
Collaborator Author

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