Skip to content

MINOR: trivial cleanups#6767

Merged
hachikuji merged 6 commits into
apache:trunkfrom
dongjinleekr:cleanup/201905
May 24, 2019
Merged

MINOR: trivial cleanups#6767
hachikuji merged 6 commits into
apache:trunkfrom
dongjinleekr:cleanup/201905

Conversation

@dongjinleekr

Copy link
Copy Markdown
Contributor
  1. Remove unused imports from CommandLineUtils, KafkaConfig.
  2. Add ignore case in KafkaController#process, GroupCoordinator#doSyncGroup, and GroupCoordinator#handleHeartbeat.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@ijuma

ijuma commented May 19, 2019

Copy link
Copy Markdown
Member

Thanks for the PR. Ignoring the exhaustiveness warnings can hide bugs, if we don't expect it to happen, we should throw an exception.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't need this because the Dead case is being handled already in the upstream.

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.

You mean, if (group.is(Dead)) { in the line 350, right? Then, how to suppress not-exhaustive match warning? Do you have any idea?

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.

We could raise an exception since we expect to have handled this state. I think it is better to be exhaustive with matches. In fact, I'd prefer to handle case Dead explicitly rather than a blind case _. That way the compiler will generate a warning if we add a new state and don't update this. In other words, the warning will have value.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same here

@hachikuji hachikuji left a comment

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.

Thanks @dongjinleekr . Left a minor suggestion. Otherwise, looks good.


case Dead =>
// this case is already handled, so raise exception
throw new IllegalStateException("can't reach this line")

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.

Can we provide a more useful message? These impossible states have a funny way of becoming possible over time 😉 . Maybe something like this?

throw new IllegalStateException(s"Reached unexpected condition for Dead group $groupId")

We can probably remove the comment if the exception message is already clear.

@dongjinleekr

Copy link
Copy Markdown
Contributor Author

Here is the fix. cc/ @abbccdda @hachikuji

@hachikuji hachikuji left a comment

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.

LGTM. Thanks @dongjinleekr !

@hachikuji
hachikuji merged commit eefb25d into apache:trunk May 24, 2019
case Startup =>
processStartup()
case ShutdownEventThread =>
// not handled here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't we throw an exception here too?

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.

Well, I have been still thinking about it; If we need an Exception, I will apply it in next cleanup. @abbccdda Could you tell me your opinion?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Previously we would throw an exception (MatchError) and now we ignore it. That seems worse, right?

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.

Yes, you are right. I went ahead and merged because it was being addressed in #6686.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense, we can just do it in #6686 then.

haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
Reviewers: Boyang Chen <bchen11@outlook.com>, Jason Gustafson <jason@confluent.io>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: Boyang Chen <bchen11@outlook.com>, Jason Gustafson <jason@confluent.io>
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.

4 participants