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

support/db: Add benchmark for BatchInsertBuilder #4086

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

2opremio
Copy link
Contributor

No description provided.

@2opremio 2opremio requested a review from bartekn November 17, 2021 15:37
@2opremio 2opremio force-pushed the batch-insert-benchmark branch from 4ab3e7d to de10ba3 Compare November 17, 2021 15:38
@2opremio 2opremio enabled auto-merge (squash) November 17, 2021 15:38
@2opremio 2opremio requested a review from a team November 17, 2021 15:54
@2opremio 2opremio force-pushed the batch-insert-benchmark branch 2 times, most recently from 1764255 to 641ddac Compare November 22, 2021 18:07
@2opremio 2opremio force-pushed the batch-insert-benchmark branch from 641ddac to e1d6fa5 Compare November 22, 2021 18:15
Table: sess.GetTable("people"),
MaxBatchSize: maxBatchSize,
}
b.StartTimer()
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to use b.ResetTimer() here instead of stoping and then starting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

auto-merge merged before I could resolve this, sorry.

I thought @leighmcculloch changed the repo config so that all comments had to be resolved before auto-merged kicked in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tamirms you are right, it's a better option. I will change it later on 👍

Copy link
Member

@leighmcculloch leighmcculloch Nov 22, 2021

Choose a reason for hiding this comment

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

Even though terraform does not support configuring the wait for conversations to be resolved, it appears it is being cleared on multiple repos, this one and the starlight repo. Maybe the terraform integration is clearing it on every deploy. I think this means we can't use the wait for conversations feature until integrations/terraform-provider-github#868 is resolved.

It might be worth holding off on using auto-merge until this is resolved. The GitHub terraform provider usually ships new features quickly so we might not be waiting that long.

cc @stellar/ops-team

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

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

Hello, nice test coverage, for relative newcomer into team, it wasn't apparent to understand driver for this, maybe brief mention in description on why it was needed, or issue# for context.

@2opremio 2opremio merged commit 97ddb3b into stellar:master Nov 22, 2021
@2opremio 2opremio deleted the batch-insert-benchmark branch November 22, 2021 19:37
@2opremio
Copy link
Contributor Author

2opremio commented Nov 22, 2021

@sreuland You are totally right, I should had provided more context (there are PR guidelines for it which I didn't follow, sorry). The context is trying to optimize the ingestion system. See #4022

Hopefully it will become more clear in an upcoming PR.

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.

4 participants