[BEAM-7005] Added a set of Jenkins load tests with GBK for Java in streaming mode #8247
Conversation
|
R: @lgajowy |
There was a problem hiding this comment.
It looks like the only difference is that we add streaming=true pipeline option. IMO it would be better to hold both jobs (batch & streaming) in one file (job_LoadTest_Java.groovy). I don't like the name of this file ("job_... prefix suggests it's only ONE job and seed job requires it) but it's still better not to duplicate the code imo, especially when this is a very rapidly developed pice of code.
LMK if you agree.
Other than that - please add some additional reviewer for transparency. ;)
|
@lgajowy I agree, I moved the tests to the main file. |
lgajowy
left a comment
lgajowy
left a comment
There was a problem hiding this comment.
Thanks! Another batch of suggestions. :)
| autoscalingAlgorithm: "NONE" | ||
| ] | ||
| ], | ||
| [ |
There was a problem hiding this comment.
If we add new elements to loadTestConfiguration we will still have 1 Jenkins job but 2x more steps in it. I belive we need separate loadTestConfigurations for both batch job and streaming job. Could you modify this accordingly?
Also, as we discussed offline, the loadTestConfiguration could be a "template" for the options that could further be parametrized in jobs. This will prevent repeating too much code. Please consider doing this while you apply changes. :)
There was a problem hiding this comment.
You're right, the template approach is much more elegant and maintainable. I applied your suggestions
|
Run seed job |
|
Run Load Tests Java GBK Dataflow Batch |
|
Run Load Tests Java GBK Dataflow Streaming |
|
Run seed job |
|
Run Load Tests Java GBK Dataflow Streaming |
1 similar comment
|
Run Load Tests Java GBK Dataflow Streaming |
markflyhigh
left a comment
markflyhigh
left a comment
There was a problem hiding this comment.
Thanks for adding streaming tests. LGTM in general.
| } | ||
|
|
||
|
|
||
| def loadTestJob = { scope, triggeringContext -> |
There was a problem hiding this comment.
Can we make loadTestJob more specific like batchLoadTestJob given we have streamingLoadTestJob?
|
Run seed job |
|
Run Load Tests Java GBK Dataflow Streaming |
1 similar comment
|
Run Load Tests Java GBK Dataflow Streaming |
|
Run Load Tests Java GBK Streaming Dataflow |
|
Run Load Tests Java GBK Dataflow Streaming |
|
Run standalone seed job |
|
Run Load Tests Java GBK Dataflow Streaming |
…heck if phrase trigger will work changed job name and phrase
mwalenia
force-pushed
the
BEAM-7005-GBK-streaming-load-test
branch
from
f2e8e3d to
84ddf77
Compare
|
Run seed job |
lgajowy
left a comment
lgajowy
left a comment
There was a problem hiding this comment.
Thanks! Besides the problem with phrase triggering, I added one more suggestion. Let me know if you need help with investigating what is going on with phrase triggering.
| */ | ||
| import CommonTestProperties | ||
|
|
||
| class CommonLoadTestConfiguration { |
There was a problem hiding this comment.
I wonder if it would be better if we have the CommonLoadTestConfiguration, streaming testing job and batch testing job in one file? We can create multiple jobs in one file. They could reuse CommonLoadTestConfiguration the same way but without the need to import it. The config would also remain hidden for the load tests only - after all we wouldn't want other (non load-test) jobs to be able to import this. WDYT?
|
Closing due to phrase triggering not working. @lgajowy I applied your suggestion in another PR where triggers are OK. |
4 participants
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-XXXwith 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.