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

Enable minibatcher #701

Closed
wants to merge 10 commits into from
Closed

Enable minibatcher #701

wants to merge 10 commits into from

Conversation

pschork
Copy link
Collaborator

@pschork pschork commented Aug 14, 2024

Why are these changes needed?

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@pschork pschork requested a review from ian-shim August 14, 2024 02:08
@pschork pschork marked this pull request as ready for review August 15, 2024 21:49
disperser/batcher/batcher.go Outdated Show resolved Hide resolved
disperser/batcher/batch_confirmer_test.go Outdated Show resolved Hide resolved
disperser/batcher/batchstore/minibatch_store.go Outdated Show resolved Hide resolved
disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
disperser/batcher/orchestrator.go Outdated Show resolved Hide resolved
@pschork pschork force-pushed the pschork/minibatch_enabled branch 5 times, most recently from 111409f to 2cbfeed Compare August 21, 2024 01:13
disperser/batcher/batcher.go Show resolved Hide resolved
Comment on lines -38 to -39
ChainState core.IndexedChainState
AssignmentCoordinator core.AssignmentCoordinator
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for cleaning this up!

disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
disperser/batcher/minibatcher_test.go Outdated Show resolved Hide resolved

o.finalizer.Start(ctx)

go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this loop in minibatcher (and Start method)?
Looks like we moved it here to have signalLiveness interweaved in the loop, but signaling liveness from the high level component seems sufficient

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't we need to signalLiveness on some sort of reoccurring loop where work is being done?

disperser/batcher/minibatcher.go Outdated Show resolved Hide resolved
Refactor batcher/minibatcher creation to use generic interface

Fix integration test

Fix tests

Update batcher interface type cast
Move minibatcher tablename check to earlier init phase

Make minibatcher table name optional

Add info output at startup

Lint

Remove log output

Rename MinibatchStore to Batchstore

Remove generic batch/minibatcher interface

Add orchestrator abstraction

Add metrics

Add finalizer

Refactor and consolidate encoding client/streamer init

Revert batcher to original
Move minibatcher branch logic to main

Move finalizer, encodingstreamer, chain state, assignment mgr into orchestrator

Add batchConfirmer to orchestrator

Move signalLiveness into orchestrator

Add transactionMgr startup to orchestrator

Lint

Lint

Lint

Lint
@pschork
Copy link
Collaborator Author

pschork commented Sep 9, 2024

Abandoned. RIP Minibatcher

@pschork pschork closed this Sep 9, 2024
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