Skip to content

[BEAM-5983] Create Combine load test#6985

Merged
lgajowy merged 2 commits intoapache:masterfrom
lgajowy:combine-test
Nov 16, 2018
Merged

[BEAM-5983] Create Combine load test#6985
lgajowy merged 2 commits intoapache:masterfrom
lgajowy:combine-test

Conversation

@lgajowy
Copy link
Copy Markdown
Contributor

@lgajowy lgajowy commented Nov 8, 2018

@aaltay could you take a look at this too?


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@lgajowy lgajowy requested a review from aaltay November 8, 2018 15:29
applyStepIfPresent(input, format("Step: %d", i), syntheticStep)
.apply(
format("Convert to BigInteger: %d", i), MapElements.via(new ByteValueToBigInteger()))
.apply(format("Combine: %d", i), combiner);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I need some feedback about what Combine modes should we test. Are Top.perKeyLargest and Mean.perKey combine operations enough?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's add sum and count.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Copy Markdown
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

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

Benchmark itself looks good, we could add more options.

applyStepIfPresent(input, format("Step: %d", i), syntheticStep)
.apply(
format("Convert to BigInteger: %d", i), MapElements.via(new ByteValueToBigInteger()))
.apply(format("Combine: %d", i), combiner);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's add sum and count.

}

/** Pipeline options specific for this test. */
interface Options extends LoadTestOptions {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we have some standard configs to run this load test? Perhaps a wrapper to run this with a set of parameters

I can think of:
Uniform
Normal
Onekey
Fanout 256 (could be uniform)
(+ maybe a few hot keys in a larger distribution.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree. This also applies to other tests (Gbk, CoGbk, ParDo etc). It's probably worth to add default parameter sets in a separate PR (I find it a cross-cutting concern). Wdyt?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lgajowy
Copy link
Copy Markdown
Contributor Author

lgajowy commented Nov 14, 2018

@aaltay applied your suggestions. PTAL again.

@lgajowy
Copy link
Copy Markdown
Contributor Author

lgajowy commented Nov 15, 2018

Run Java PreCommit

@lgajowy lgajowy merged commit fa1142c into apache:master Nov 16, 2018
@lgajowy lgajowy deleted the combine-test branch November 16, 2018 09:40
@lgajowy
Copy link
Copy Markdown
Contributor Author

lgajowy commented Nov 16, 2018

Thanks!

ajamato pushed a commit to ajamato/beam that referenced this pull request Nov 27, 2018
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