-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Bug][KafkaSource]Fix the default value of commit_on_checkpoint. #3831
Conversation
@@ -38,7 +38,7 @@ public class ConsumerMetadata implements Serializable { | |||
private String bootstrapServers; | |||
private Properties properties; | |||
private String consumerGroup; | |||
private boolean commitOnCheckpoint = false; | |||
private boolean commitOnCheckpoint = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value need get from the COMMIT_ON_CHECKPOINT
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use COMMIT_ON_CHECKPOINT
to define the default value.
public static final Option<Boolean> COMMIT_ON_CHECKPOINT = Options.key("commit_on_checkpoint")
.booleanType()
.defaultValue(true)
.withDescription("If true the consumer's offset will be periodically committed in the background.");
So, I think if user does not set it in config file, you need get the default value from COMMIT_ON_CHECKPOINT.defaultValue()
. Otherwise, the defaultValue in Option will lose its meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, you're right. It's been changed.
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@EricJoy2048 please merge, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
…che#3831) * Fix the default value of commit_on_checkpoint
…che#3831) * Fix the default value of commit_on_checkpoint
Fix the default value of commit_on_checkpoint,it is different from the default value described in the document, and the modification is consistent.
Purpose of this pull request
Check list
New License Guide