Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: clean up Streams assignment classes and tests #8406

Merged
merged 3 commits into from
Apr 3, 2020

Conversation

ableegoldman
Copy link
Contributor

@ableegoldman ableegoldman commented Apr 2, 2020

First set of cleanup pushed to followup PR after KIP-441 Pt. 5. Main changes are:

  1. Moved RankedClient and the static buildClientRankingsByTask to a new file
  2. Moved Movement and the static getMovements to a new file (also renamed to TaskMovement)
  3. Consolidated the many common variables throughout the assignment tests to the new AssignmentTestUtils
  4. New utility to generate comparable/predictable UUIDs for tests, and removed the generic from TaskAssignor and all related classes

Keep in mind, this does not represent the sum whole of all cleanup we'd like to do. But if there's anything you had in mind that you feel would fit in with the other changes in this PR, feel free to point it out

@ableegoldman ableegoldman force-pushed the KIP-441-clean-up-HATA-class branch 3 times, most recently from 77baa5d to aee4dd6 Compare April 2, 2020 19:14
@vvcephei
Copy link
Contributor

vvcephei commented Apr 2, 2020

test this please

Copy link
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!


/**
* Builds a UUID by repeating the given number n. For valid n, it is guaranteed that the returned UUIDs satisfy
* the same relation relative to others as their parameter n does: iff n < m, then uuidForInt(n) < uuidForInt(m)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just important to produce stable test results for verification?

@vvcephei
Copy link
Contributor

vvcephei commented Apr 2, 2020

Failed due to github outage.

Retest this, please

@vvcephei
Copy link
Contributor

vvcephei commented Apr 3, 2020

Hey @ableegoldman , looks like there are some problems:

java.lang.Exception: No public static parameters method on class org.apache.kafka.streams.processor.internals.StreamsPartitionAssignorTest


java.lang.ClassCastException: class java.util.UUID cannot be cast to class java.lang.Integer (java.util.UUID and java.lang.Integer are in module java.base of loader 'bootstrap')
	at java.base/java.lang.Integer.compareTo(Integer.java:59)
	at java.base/java.util.TreeMap.getEntry(TreeMap.java:350)
	at java.base/java.util.TreeMap.get(TreeMap.java:277)
	at org.apache.kafka.streams.processor.internals.assignment.StickyTaskAssignorTest.shouldNotHaveSameAssignmentOnAnyTwoHosts(StickyTaskAssignorTest.java:418)

@vvcephei
Copy link
Contributor

vvcephei commented Apr 3, 2020

test this please

1 similar comment
@vvcephei
Copy link
Contributor

vvcephei commented Apr 3, 2020

test this please

@vvcephei vvcephei merged commit 6e0d553 into apache:trunk Apr 3, 2020
@ableegoldman ableegoldman deleted the KIP-441-clean-up-HATA-class branch June 26, 2020 22:37
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.

3 participants