Skip to content

Fix unrealistic test variables in KafkaSupervisorTest and tidy up unused variable in checkpointing process#7319

Merged
jihoonson merged 9 commits intoapache:masterfrom
jihoonson:fix-kafka-supervisor-test
Aug 21, 2019
Merged

Fix unrealistic test variables in KafkaSupervisorTest and tidy up unused variable in checkpointing process#7319
jihoonson merged 9 commits intoapache:masterfrom
jihoonson:fix-kafka-supervisor-test

Conversation

@jihoonson
Copy link
Copy Markdown
Contributor

The start sequence numbers would never have exclusivePartitions in Kafka. Having exclusivePartitions in unit tests wouldn't harm, but would never happen in production. Also, the second parameter for checkpoint() is not being used. I removed it.

Since this is not a bug fix, but just tidying up unused variables and test variables, it doesn't have to be backported.

@jihoonson jihoonson changed the title Fix unrealistic test variables in KafkaSupervisorTest and tidy up unused variable Fix unrealistic test variables in KafkaSupervisorTest and tidy up unused variable in checkpointing process Mar 21, 2019
@fjy
Copy link
Copy Markdown
Contributor

fjy commented May 15, 2019

@jihoonson mind fixing conflicts?

@jihoonson
Copy link
Copy Markdown
Contributor Author

@fjy sure fixed.

@Deprecated String baseSequenceName,
SeekableStreamDataSourceMetadata<PartitionIdType, SequenceOffsetType> previousCheckpoint,
SeekableStreamDataSourceMetadata<PartitionIdType, SequenceOffsetType> currentCheckpoint
SeekableStreamDataSourceMetadata<PartitionIdType, SequenceOffsetType> previousCheckpoint
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should previousCheckpoint be renamed to checkpointMetadata ?

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, fixed.

@stale
Copy link
Copy Markdown

stale Bot commented Jul 15, 2019

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

@stale stale Bot added the stale label Jul 15, 2019
@jihoonson
Copy link
Copy Markdown
Contributor Author

Will fix this.

@stale
Copy link
Copy Markdown

stale Bot commented Jul 15, 2019

This issue is no longer marked as stale.

@stale stale Bot removed the stale label Jul 15, 2019
Copy link
Copy Markdown

@surekhasaharan surekhasaharan left a comment

Choose a reason for hiding this comment

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

LGTM!

@jihoonson jihoonson merged commit 22d6384 into apache:master Aug 21, 2019
@jihoonson
Copy link
Copy Markdown
Contributor Author

@fjy @clintropolis @surekhasaharan thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants