-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[IcebergIO] load tests #33728
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
base: master
Are you sure you want to change the base?
[IcebergIO] load tests #33728
Conversation
8693e5b to
e894a86
Compare
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
|
Assigning reviewers: R: @robertwb for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
|
||
| int multiplier = this.startMultiplier; | ||
| long elapsedTimeMillis = timestamp.getMillis() - startTimesMillis; | ||
| long elapsedTimeMillis = Instant.now().getMillis() - startTimesMillis; |
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 are you trying to accomplish here? One side effect of this change is that now 2 different load tests will send different volumes of messages (e.g. if I'm slow processing messages, I'll get extra messages because more of them will have late timestamps).
I'm not super familiar with this code, so possible I'm misunderstanding the previous behavior, but growing load like this seems like it could be suboptimal
| readPipeline | ||
| .getOptions() | ||
| .as(DataflowPipelineOptions.class) | ||
| .setNumWorkers(configuration.numWorkers / 5); |
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.
Why is read so much lower than write?
|
|
||
| /** | ||
| * Determines whether to use Dataflow runner v2. If set to true, it uses SDF mode for reading | ||
| * from Kafka. Otherwise, Unbounded mode will be used. |
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.
I think this comment needs updated
|
waiting on author |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
Hi @ahmedabu98, is this PR still valid? Thanks! |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
not stale |
No description provided.