Skip to content

MINOR: Fix bugs identified by compiler warnings#6258

Merged
ijuma merged 10 commits intoapache:trunkfrom
ijuma:fix-compiler-warnings
Feb 15, 2019
Merged

MINOR: Fix bugs identified by compiler warnings#6258
ijuma merged 10 commits intoapache:trunkfrom
ijuma:fix-compiler-warnings

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Feb 12, 2019

  • Add missing string interpolation
  • Fix and simplify testElectPreferredLeaders
  • Remove unused code
  • Replace deprecated usage of JUnit assertThat
  • Change var to val and fix non-exhaustive pattern match
  • Fix eta warning
  • Simplify code
  • Remove commented out code

Committer Checklist (excluded from commit message)

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@tombentley Please check that we didn't intend to use this.

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, this can be removed since the logic is folded into onComplete() already.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@junrao Please check that this is OK.

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.

@ijuma : Yes, this part looks good to me.

Copy link
Copy Markdown
Member Author

@ijuma ijuma Feb 12, 2019

Choose a reason for hiding this comment

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

@tombentley after I fixed this, the test started failing. return was causing everything after this not to run. Can you please check what the issue is and fix it? cc @mjsax in case this is a regression and not a test problem.

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.

@ijuma : Good find. These are indeed existing bugs. There are 4 references of LeaderNotAvailableException in line 1358, 1372, 1380 and 1390. They all need to be changed to PreferredLeaderNotAvailableException. Once I fixed those locally, the test passes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks Jun! Will fix.

@ijuma ijuma force-pushed the fix-compiler-warnings branch from c6c610d to f2b655d Compare February 14, 2019 03:43
@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Feb 14, 2019

@junrao I pushed a fix for the failing test and minor tweaks. Let me know if this looks good to you.

Copy link
Copy Markdown
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@ijuma : Thanks for the patch. Great cleanup! LGTM

@ijuma ijuma merged commit a421dd2 into apache:trunk Feb 15, 2019
@ijuma ijuma deleted the fix-compiler-warnings branch February 15, 2019 01:13
@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Feb 15, 2019

Merged to trunk and cherry-picked to 2.2.

jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
* ak/trunk: (45 commits)
  KAFKA-7487: DumpLogSegments misreports offset mismatches (apache#5756)
  MINOR: improve JavaDocs about auto-repartitioning in Streams DSL (apache#6269)
  KAFKA-7935: UNSUPPORTED_COMPRESSION_TYPE if ReplicaManager.getLogConfig returns None (apache#6274)
  KAFKA-7895: Fix stream-time reckoning for suppress (apache#6278)
  KAFKA-6569: Move OffsetIndex/TimeIndex logger to companion object  (apache#4586)
  MINOR: add log indicating the suppression time (apache#6260)
  MINOR: Make info logs for KafkaConsumer a bit more verbose (apache#6279)
  KAFKA-7758: Reuse KGroupedStream/KGroupedTable with named repartition topics (apache#6265)
  KAFKA-7884; Docs for message.format.version should display valid values (apache#6209)
  MINOR: Save failed test output to build output directory
  MINOR: add test for StreamsSmokeTestDriver (apache#6231)
  MINOR: Fix bugs identified by compiler warnings (apache#6258)
  KAFKA-6474: Rewrite tests to use new public TopologyTestDriver [part 4] (apache#5433)
  MINOR: fix bypasses in ChangeLogging stores (apache#6266)
  MINOR: Make MockClient#poll() more thread-safe (apache#5942)
  MINOR: drop dbAccessor reference on close (apache#6254)
  KAFKA-7811: Avoid unnecessary lock acquire when KafkaConsumer commits offsets (apache#6119)
  KAFKA-7916: Unify store wrapping code for clarity (apache#6255)
  MINOR: Add missing Alter Operation to Topic supported operations list in AclCommand
  KAFKA-7921: log at error level for missing source topic (apache#6262)
  ...
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
- Add missing string interpolation
- Fix and simplify testElectPreferredLeaders
- Remove unused code
- Replace deprecated usage of JUnit `assertThat`
- Change var to val and fix non-exhaustive pattern match
- Fix eta warning
- Simplify code
- Remove commented out code

Reviewers: Jun Rao <junrao@gmail.com>
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