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

Always flush when timer goes off #6563

Merged
merged 3 commits into from
Jun 23, 2021
Merged

Always flush when timer goes off #6563

merged 3 commits into from
Jun 23, 2021

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Jun 22, 2021

Might add a test, but here's a PR.

Fixes #6541

@arajasek arajasek requested a review from magik6k as a code owner June 22, 2021 23:21
@arajasek arajasek force-pushed the asr/batch-starter branch from 854e07c to 93f7cbe Compare June 22, 2021 23:31
@BigLep
Copy link
Member

BigLep commented Jun 23, 2021

Is there a reason we wouldn't add a test here?

@BigLep BigLep linked an issue Jun 23, 2021 that may be closed by this pull request
Copy link
Member

@jennijuju jennijuju left a comment

Choose a reason for hiding this comment

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

I think the same logic change should be applied to pre commit batcher too.

@BigLep BigLep requested a review from a team June 23, 2021 06:10
@magik6k
Copy link
Contributor

magik6k commented Jun 23, 2021

I think the same logic change should be applied to pre commit batcher too.

In the precommit batcher the min batch size has a slightly different meaning as we haven't committed to publishing those sectors yet, and sending individual precommits makes no sense, so the only use for MinBatch is not sending anything

We might want to have Commit / PreCommitMinBatch config vars have different names tho, to make sure it's clear that they are doing quite different things

@arajasek
Copy link
Contributor Author

I think the same logic change should be applied to pre commit batcher too.

In the precommit batcher the min batch size has a slightly different meaning as we haven't committed to publishing those sectors yet, and sending individual precommits makes no sense, so the only use for MinBatch is not sending anything

We might want to have Commit / PreCommitMinBatch config vars have different names tho, to make sure it's clear that they are doing quite different things

Aren't there two cases when we do want to publish sectors, though:

  • if a ticket is expiring (and we're risking losing the PC work)
  • if a deal included in the sector is approaching activation

@arajasek
Copy link
Contributor Author

Summary: makes sense to just remove MinPreCommitBatch entirely -- if the timer goes off, we just batch and precommit whatever we have.

Thanks @jennijuju!

@magik6k magik6k merged commit 0e4cd3c into release/v1.10.0 Jun 23, 2021
@magik6k magik6k deleted the asr/batch-starter branch June 23, 2021 17:19
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.

Send commits using ProveCommitSectors when time configs hit
4 participants