Skip to content

KAFKA-7080 and KAFKA-7222: Cleanup overlapping KIP changes Part 2#5806

Closed
vvcephei wants to merge 2 commits intoapache:trunkfrom
vvcephei:fix-missing-segment-interval
Closed

KAFKA-7080 and KAFKA-7222: Cleanup overlapping KIP changes Part 2#5806
vvcephei wants to merge 2 commits intoapache:trunkfrom
vvcephei:fix-missing-segment-interval

Conversation

@vvcephei
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei commented Oct 16, 2018

#5804 removed Windows#segmentInterval, but did not remove all references to it.

Committer Checklist (excluded from commit message)

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

@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Oct 16, 2018

@guozhangwang @mjsax @bbejeck @stanislavkozlovski ,

This PR is to fix the broken trunk build.

It also needs to be included in 2.1

@dguy
Copy link
Copy Markdown
Contributor

dguy commented Oct 16, 2018

@vvcephei does RocksDBWindowStoreTest also need to be updated?

vvcephei
vvcephei commented Oct 16, 2018
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBWindowStoreTest.java

private WindowStore<Integer, String> createWindowStore(final ProcessorContext context, final boolean retainDuplicates) {
final WindowStore<Integer, String> store = Stores.windowStoreBuilder(
Stores.persistentWindowStore(windowName, retentionPeriod, windowSize, retainDuplicates, segmentInterval),
Copy link
Copy Markdown
Contributor Author

@vvcephei vvcephei Oct 16, 2018

Choose a reason for hiding this comment

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

@dguy Yes, this is where I've updated it.

Copy link
Copy Markdown
Contributor

@dguy dguy Oct 16, 2018

Choose a reason for hiding this comment

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

I think that method is private?? I might have missed somerhing

Copy link
Copy Markdown
Contributor Author

@vvcephei vvcephei Oct 16, 2018

Choose a reason for hiding this comment

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

It was previously public. #5804 made it private, but did not remove this reference. This is why trunk fails to build right now.

I'm removing this reference and instead calling a public method.

@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Oct 16, 2018

Java 8 failure:
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup: java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.CoordinatorNotAvailableException: The coordinator is not available.

Java 11 failure:
kafka.log.LogCleanerIntegrationTest.testMarksPartitionsAsOfflineAndPopulatesUncleanableMetrics: org.junit.runners.model.TestTimedOutException: test timed out after 15000 milliseconds

Retest this, please.

@rondagostino rondagostino mentioned this pull request Oct 17, 2018
Merged
@kamalcph kamalcph mentioned this pull request Oct 17, 2018
Closed
3 tasks
@kamalcph
Copy link
Copy Markdown
Contributor

kamalcph commented Oct 17, 2018

@vvcephei @mjsax

Please review the doc / code committed in #5804

Finally, Stores#persistentWindowStore(...) were deprecated and replaced with a new overload that does not allow to specify the number of segments any longer.

It states that numSegments parameter no longer accepted but the method still accepts it.

@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented Oct 17, 2018

@kamalcph The docs say it was deprecated, not removed, and the the single old persistentWindowStore method has indeed been marked deprecated.

@ewencp ewencp mentioned this pull request Oct 17, 2018
Merged
@ewencp
Copy link
Copy Markdown
Contributor

ewencp commented Oct 17, 2018

Merging this to trunk to unblock everything else since the trunk build is broken currently. I ran the tests locally, looks like we have just accumulated a lot of flaky tests that are especially flaky on jenkins.

@ewencp ewencp closed this in eae5ae3 Oct 17, 2018
@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Oct 17, 2018

@ewencp thank you!

@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Oct 17, 2018

Thanks, @ewencp!

@vvcephei vvcephei deleted the fix-missing-segment-interval branch October 17, 2018 20:09
@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Oct 17, 2018

@ewencp The 2.1 branch was also broken by #5804 (64ce84c). Do you mind cherry-picking this fix (eae5ae3) over to 2.1?

@vvcephei
Copy link
Copy Markdown
Contributor Author

vvcephei commented Oct 17, 2018

Or maybe @guozhangwang can

guozhangwang pushed a commit that referenced this pull request Oct 17, 2018
@vvcephei @guozhangwang
KAFKA-7080 and KAFKA-7222: Cleanup overlapping KIP changes Part 2
bb370d2 
#5804 removed `Windows#segmentInterval`, but did not remove all references to it.

Author: John Roesler <john@confluent.io>

Reviewers: Damian Guy <damian.guy@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes #5806 from vvcephei/fix-missing-segment-interval
@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Oct 17, 2018

I've pushed to 2.1 as well

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Oct 18, 2018

Thanks @vvcephei @dguy @ewencp @guozhangwang

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@vvcephei @pengxiaolong
KAFKA-7080 and KAFKA-7222: Cleanup overlapping KIP changes Part 2
eff8062 
apache#5804 removed `Windows#segmentInterval`, but did not remove all references to it.

Author: John Roesler <john@confluent.io>

Reviewers: Damian Guy <damian.guy@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io>

Closes apache#5806 from vvcephei/fix-missing-segment-interval
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dguy dguy dguy left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vvcephei @dguy @kamalcph @ewencp @guozhangwang @mjsax