Skip to content

Splitting KafkaIndexTask for better code maintenance#5854

Merged
jihoonson merged 8 commits intoapache:masterfrom
jihoonson:kafka-task-cleanup
Jun 22, 2018
Merged

Splitting KafkaIndexTask for better code maintenance#5854
jihoonson merged 8 commits intoapache:masterfrom
jihoonson:kafka-task-cleanup

Conversation

@jihoonson
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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