-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-5980] Remove redundant combine tests #9286
[BEAM-5980] Remove redundant combine tests #9286
Conversation
Run seed job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK, but there is also Flink test which requires changing. I know there are works started in Flink tests refactor, but I am afraid it will be forgotten.
You are right - I will remove there too. |
Run seed job |
It seems that I didn't mess up the jobs. Could you take a look again @kkucharc ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks OK 👍 I left one more comment to be checked.
} | ||
|
||
infra.scaleCluster(scope, jenkinsJobName, scaledNumberOfWorkers) | ||
def numberOfWorkers = 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about making it global again and put in parallelism param in each test configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
ce8be07
to
c02a05a
Compare
Run seed job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks Ok :) Maybe let's run Flink test from PR, this one had 'more advanced' changes, WDYT?
Let's wait unitl monday with this PR. There's ongoing discussion in the proposal whether all of them should be deleted or not |
LGTM. Feel free to self-merge if necessary. |
c02a05a
to
bb839a6
Compare
Run seed job |
I left #1 test not deleted as @angoenka suggested. Removing only #2 and #3 test now. See the discussion: https://docs.google.com/a/polidea.com/document/d/1PuIQv4v06eosKKwT76u7S6IP88AnXhTf870Rcj1AHt4/edit?disco=AAAADZUGzAI |
@mwalenia could you take a quick look as well? |
@@ -32,7 +33,7 @@ String pythonHarnessImageTag = "${dockerRegistryRoot}/python:${dockerTag}" | |||
String flinkVersion = '1.7' | |||
String flinkDownloadUrl = 'https://archive.apache.org/dist/flink/flink-1.7.0/flink-1.7.0-bin-hadoop28-scala_2.11.tgz' | |||
|
|||
def loadTestConfigurationsFiveWorkers = { datasetName -> [ | |||
def scenarios = { datasetName -> [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that datasetName
is not used. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is used to fill metrics_dataset
according to triggering context (pr/not a pr).
LGTM, thanks, @lgajowy ! |
bb839a6
to
819c61f
Compare
It turned out we tested nothing relevant with the additional 2 tests that are deleted. The Combine test maps values of different sizes to Long value. The deleted tests had only different initial size so, in practice, they were testing the same thing as test #1.
819c61f
to
830e99b
Compare
Run seed job |
I squashed some commits... |
Size of input records is irrelevant for the combine operation because the byte[] value gets mapped to a Long before the Combine actually happens. This makes the tests redundant as size is the only thing that changes in the synthetic source options given to the input.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.