Skip to content

MINOR: Remove usages of scala.collection.JavaConversions, and some typo fixes#5115

Merged
ijuma merged 5 commits into
apache:trunkfrom
vahidhashemian:minor/typo-fix-1806
Jun 3, 2018
Merged

MINOR: Remove usages of scala.collection.JavaConversions, and some typo fixes#5115
ijuma merged 5 commits into
apache:trunkfrom
vahidhashemian:minor/typo-fix-1806

Conversation

@vahidhashemian

@vahidhashemian vahidhashemian commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

Committer Checklist (excluded from commit message)

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

@vahidhashemian vahidhashemian changed the title MINOR: Removed usages of scala.collection.JavaConversions MINOR: Remove usages of scala.collection.JavaConversions Jun 1, 2018

@ijuma ijuma left a comment

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.

Thanks for the PR. One comment. Also worth mentioning the typos in the title since that ends up being most of the PR.

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.

Doesn't seem like calling toMap on every entry is a good idea.

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.

Is it on every entry? or once the whole props object?
In any case, I refactored it to look more like Scala.

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.

My bad, I misread the code.

@vahidhashemian vahidhashemian changed the title MINOR: Remove usages of scala.collection.JavaConversions MINOR: Remove usages of scala.collection.JavaConversions, and some typo fixes Jun 2, 2018

@vahidhashemian vahidhashemian left a comment

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.

@ijuma thanks for the feedback. Updated the title in the new commit.
Not sure if I got your point on the inline comment. Made an update anyway, please take a look. Thanks.

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.

Is it on every entry? or once the whole props object?
In any case, I refactored it to look more like Scala.

@ijuma ijuma left a comment

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.

I pushed a couple of trivial changes. LGTM after that. @vahidhashemian, do they look OK to you too?

@ijuma

ijuma commented Jun 2, 2018

Copy link
Copy Markdown
Member

Actually, it seems like we still use JavaConversions in InternalTopicIntegrationTest, KafkaEmbedded and EmbeddedKafkaCluster. Any reason why we can't replace those @vahidhashemian?

@vahidhashemian

Copy link
Copy Markdown
Contributor Author

@ijuma, your updates look good. I had missed those other ones in the Java code. Removed them in the new commit.

@ijuma

ijuma commented Jun 3, 2018

Copy link
Copy Markdown
Member

Retest this please

@vahidhashemian

Copy link
Copy Markdown
Contributor Author

I think the failure was related to this hotfix. I did a rebase.

@vahidhashemian

Copy link
Copy Markdown
Contributor Author

Retest this please

@vahidhashemian

Copy link
Copy Markdown
Contributor Author

Sorry, forgot to pull your earlier commit :(

@ijuma ijuma left a comment

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.

LGTM

@ijuma
ijuma merged commit 0cacbcf into apache:trunk Jun 3, 2018
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
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.

2 participants