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

Splitting KafkaIndexTask for better code maintenance #5854

Merged
merged 8 commits into from
Jun 22, 2018

Conversation

jihoonson
Copy link
Contributor

@jihoonson jihoonson commented Jun 7, 2018

Fixes #5771.

Added KafkaIndexTaskRunner and its two implementations for the legacy and the recent one. Please note that the goal of this PR is to make the non-legacy code of KafkaIndexTask more understandable. There ARE a bunch of code duplication, but I think this is better than refactoring including a good abstraction.

  • LegacyKafkaIndexTaskRunner is simply a copy of KafkaIndexTask.runInternalLegacy() and required variables. LegacyKafkaIndexTaskRunner is expected to be removed in the future, so there is no need to put effort to make the code structure better. Also, it's easier to remove the legacy code in the future.
  • There are already a bunch of code duplication in KafkaIndexTask between the legacy mode and the incremental publishing mode. A good abstraction between them requires rewriting lots of codes.

This change is Reviewable

@jihoonson jihoonson changed the title Refactoring KafkaIndexTask for better code maintenance Splitting KafkaIndexTask for better code maintenance Jun 7, 2018
import javax.annotation.Nullable;
import java.util.Map;

final class SetEndOffsetsResult
Copy link
Contributor

Choose a reason for hiding this comment

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

this class is unused, let's remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Removed it.

@jihoonson jihoonson merged commit 8c5ded0 into apache:master Jun 22, 2018
@jihoonson
Copy link
Contributor Author

@jon-wei thank you 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.

3 participants