Break batch event storage into smaller chunks #122

ramtinms · 2020-11-05T17:50:16Z

Badger has a limit on the size of a transaction, by breaking events into smaller chunks we can prevent this error when many events are produced.

could not save execution results: failed to store events: could not insert event: could not store data: Txn is too big to fit into one request

zhangchiqing · 2020-11-05T19:55:27Z

engine/execution/ingestion/engine.go

+				if end > len(events) {
+					end = len(events)
+				}
+				err = e.events.Store(blockID, events[i:end])


Could we make a function chunkfiyEvents(events []Event, chunkSize) [][]Event, and write some tests for that?

just applied the changes

…flow/flow-go into ramtin/improvements-to-result-storage

jordanschalm · 2020-11-09T23:07:43Z

engine/execution/ingestion/engine.go

-			err = e.events.Store(blockID, events)
+		// store events 1K in each batch
+		chunkSize := uint(1000)
+		eventChunks := ChunkfiyEvents(events, chunkSize)


Suggested change

eventChunks := ChunkfiyEvents(events, chunkSize)

eventChunks := ChunkifyEvents(events, chunkSize)

fixed at cfc8bcf

jordanschalm · 2020-11-09T23:11:56Z

engine/execution/ingestion/engine_test.go

+	events := make([]flow.Event, 0)
+	for j := 0; j < 10; j++ {
+		events[j] = unittest.EventFixture(flow.EventAccountCreated, uint32(j), uint32(j), unittest.IdentifierFixture())
+	}


Suggested change

events := make([]flow.Event, 0)

for j := 0; j < 10; j++ {

events[j] = unittest.EventFixture(flow.EventAccountCreated, uint32(j), uint32(j), unittest.IdentifierFixture())

}

var events []flow.Event

for j := 0; j < 10; j++ {

events = append(events, unittest.EventFixture(flow.EventAccountCreated, uint32(j), uint32(j), unittest.IdentifierFixture()))

}

I think the way it is now will cause index out of bound error

fixed at cfc8bcf

jordanschalm · 2020-11-09T23:12:12Z

engine/execution/ingestion/engine_test.go

@@ -875,3 +875,40 @@ func TestLoadingUnexecutedBlocks(t *testing.T) {
 			pending)
 	})
 }
+
+func TestChunkfiyEvents(t *testing.T) {


Suggested change

func TestChunkfiyEvents(t *testing.T) {

func TestChunkifyEvents(t *testing.T) {

fixed at cfc8bcf

jordanschalm · 2020-11-09T23:14:12Z

ledger/complete/wal/checkpointer_test.go

@@ -179,6 +180,8 @@ func Test_Checkpointing(t *testing.T) {

 				savedData[string(rootHash)] = data
 			}
+			// some buffer time of the checkpointer to run
+			time.Sleep(1 * time.Second)


Is this to address flakey tests? I'm not too familiar with how this works, but maybe we can expose a way to force the checkpointer to run and wait for it synchronously.

jordanschalm · 2020-11-09T23:15:50Z

engine/execution/ingestion/engine.go

-		if len(events) > 0 {
-			err = e.events.Store(blockID, events)
+		// store events 1K in each batch
+		chunkSize := uint(1000)


Do you know if the transaction limit in Badger is metered by DB operation or by data size? If we have really large events will this still work?

seems to be a limit based on the ratio of the table size https://discuss.dgraph.io/t/discussion-badgerdb-should-offer-arbitrarily-sized-atomic-transactions/8736 :-|
we might have to change the "chunkify" to act based on the event size.

Further down that discussion they point to https://github.com/dgraph-io/badger/blob/ef28ef36b5923f12ffe3a1702bdfa6b479db6637/batch.go#L37-L42

That might be a good fit for this.

but considering the event byte size limit that we are going to put in place, I think we can come up with some limits for tx based on the counts.

The issue with have with the batch writer is we do look up for IDs I guess before writing, we might change it completely to use the batch writer and do upsert style of storage for events

The issue with have with the batch writer is we do look up for IDs I guess before writing

I think to use the batch writer you anyway need to re-write that operation level function to call batch.Set rather than tx.Set. But you're right it would change the semantics.

@jordanschalm do you there is a value in merging this PR or we should create an issue to properly address this issue by size ?

I'm good to merge this. It seems like it will only make the problem better, even if it doesn't fully handle all cases. But we should make an issue to implement a solution that uses BatchWriter or that guarantees we won't hit the limits implicitly with event/tx size/count limits (I prefer BatchWriter though).

ramtinms · 2020-12-02T00:13:19Z

what do you think @zhangchiqing

The diff contains: - a lot of changes that concern parts of the library we don't use (other curves a la BN, BLS24-X, BLS12-383 ...), integer protocols (ETRS), field extension machinery ... - otherwise irrelevant changes, e.g. CI/CD - some memory bug fixing [Full Changeset](https://github.com/relic-toolkit/relic/compare/7a9bba7f..9206ae5) **Fixed bugs:** - Unexpected failure of ep2\_mul\[\_lwnaf\] above the prime group order [\onflow#64](relic-toolkit/relic#64) **Closed issues:** - Other way to construct towered extension fields [\onflow#203](relic-toolkit/relic#203) - blake2.h:101:5: error: size of array element is not a multiple of its alignment [\onflow#202](relic-toolkit/relic#202) - ECIES 160bit [\onflow#201](relic-toolkit/relic#201) - Compilation with "ARITH gmp" fails [\onflow#200](relic-toolkit/relic#200) - Support for armv8-a ? [\onflow#198](relic-toolkit/relic#198) - Function name bn\_init conflicts with OpenSSL when used in tandem [\onflow#196](relic-toolkit/relic#196) - 16-bit MSP430 [\onflow#193](relic-toolkit/relic#193) - Modular exponentiation returns 1 if exponent is 0 and modulo is 1 [\onflow#185](relic-toolkit/relic#185) - Compilation of RELIC with bls12-446 and bls12-455 fails [\onflow#182](relic-toolkit/relic#182) - test\_bn fails with BLS12-381 preset [\onflow#181](relic-toolkit/relic#181) - \[BUG\] undefined reference to `bench_init', `bench\_clean' [\onflow#180](relic-toolkit/relic#180) - Tests FTBFS because of missing symbol in header [\onflow#179](relic-toolkit/relic#179) - Builds are broken [\onflow#178](relic-toolkit/relic#178) - compile error inlining failed in call to always\_inline ‘\_mm\_alignr\_epi8’ on unbantu20.04 gcc9 [\onflow#177](relic-toolkit/relic#177) - bn\_write\_str buffer overflow [\onflow#176](relic-toolkit/relic#176) - ECDSA verify succeeds when it should fail [\onflow#175](relic-toolkit/relic#175) - ec\_mul\_gen hangs with curve SECG\_K256 [\onflow#174](relic-toolkit/relic#174) - Wrong square root computation [\onflow#173](relic-toolkit/relic#173) - Out-of-bounds read via bn\_sqr\_basic [\onflow#172](relic-toolkit/relic#172) - OSS-Fuzz integration [\onflow#171](relic-toolkit/relic#171) - Building Relic with Curve NIST\_P256 throws FATAL ERROR in relic\_fp\_prime.c:120 [\onflow#170](relic-toolkit/relic#170) - Compressing \(packing\) a point to binary array does not comply with X9.62 standard [\onflow#169](relic-toolkit/relic#169) - ‘ctx\_t’ {aka ‘struct \_ctx\_t’} has no member named ‘total’ [\onflow#168](relic-toolkit/relic#168) - relic does not work with C++ [\onflow#167](relic-toolkit/relic#167) - Memory leak in ep2\_curve\_init/clean with ALLOC=DYNAMIC [\onflow#166](relic-toolkit/relic#166) - \*\_is\_valid\(\) functions produce false negative for not normalized points [\onflow#147](relic-toolkit/relic#147) - Bench and Test doesnt build [\onflow#122](relic-toolkit/relic#122) **Merged pull requests:** - Add pairing delegation protocols [\onflow#199](relic-toolkit/relic#199) ([dfaranha](https://github.com/dfaranha)) - Fix support for Win64/MSVC targets. [\onflow#197](relic-toolkit/relic#197) ([dfaranha](https://github.com/dfaranha)) - Simplify generator getting for Gt. [\onflow#194](relic-toolkit/relic#194) ([luozejiaqun](https://github.com/luozejiaqun)) - cmake: Always use user defined CFLAGS, not only for release builds [\onflow#187](relic-toolkit/relic#187) ([xdustinface](https://github.com/xdustinface)) - Fix MinGW build [\onflow#186](relic-toolkit/relic#186) ([xdustinface](https://github.com/xdustinface)) - Remove debug printf in bn\_mxp\_slide [\onflow#184](relic-toolkit/relic#184) ([guidovranken](https://github.com/guidovranken)) - Remove ALLOC = STACK to simplify memory allocation. [\onflow#183](relic-toolkit/relic#183) ([dfaranha](https://github.com/dfaranha)) - Update relic\_alloc.h [\onflow#165](relic-toolkit/relic#165) ([aguycalled](https://github.com/aguycalled)) - Add correct support for FreeBSD and NetBSD [\onflow#164](relic-toolkit/relic#164) ([hoffmang9](https://github.com/hoffmang9))

chunk the events into smaller batches

d59f006

ramtinms requested review from m4ksio and psiemens as code owners November 5, 2020 17:50

ramtinms requested a review from zhangchiqing November 5, 2020 17:50

zhangchiqing reviewed Nov 5, 2020

View reviewed changes

ramtinms and others added 4 commits November 9, 2020 13:01

chunkify events

5e47c94

Merge branch 'master' into ramtin/improvements-to-result-storage

a9f19cc

fix lint

8804f1b

Merge branch 'ramtin/improvements-to-result-storage' of github.com:on…

0a7a9fe

…flow/flow-go into ramtin/improvements-to-result-storage

ramtinms requested review from zhangchiqing, jordanschalm and Kay-Zee November 9, 2020 21:48

hopefully this fixes the flaky test

dee336c

ramtinms requested a review from AlexHentschel as a code owner November 9, 2020 22:05

jordanschalm reviewed Nov 9, 2020

View reviewed changes

ramtinms and others added 4 commits November 9, 2020 15:39

applied PR comments

cfc8bcf

fix the flaky test

d6ab704

Merge branch 'master' into ramtin/improvements-to-result-storage

aa167c1

Merge branch 'master' into ramtin/improvements-to-result-storage

b723443

jordanschalm approved these changes Nov 20, 2020

View reviewed changes

Merge branch 'master' into ramtin/improvements-to-result-storage

30670ee

Merge branch 'master' into ramtin/improvements-to-result-storage

a9ff7db

zhangchiqing approved these changes Dec 2, 2020

View reviewed changes

ramtinms added 3 commits December 2, 2020 10:13

Merge branch 'master' into ramtin/improvements-to-result-storage

37710fb

Merge branch 'master' into ramtin/improvements-to-result-storage

9f2790f

Merge branch 'master' into ramtin/improvements-to-result-storage

797d0eb

ramtinms merged commit 6544f8d into master Dec 3, 2020

ramtinms deleted the ramtin/improvements-to-result-storage branch December 3, 2020 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break batch event storage into smaller chunks #122

Break batch event storage into smaller chunks #122

ramtinms commented Nov 5, 2020

zhangchiqing Nov 5, 2020

ramtinms Nov 9, 2020

jordanschalm Nov 9, 2020

ramtinms Nov 9, 2020

jordanschalm Nov 9, 2020

ramtinms Nov 9, 2020

jordanschalm Nov 9, 2020

ramtinms Nov 9, 2020

jordanschalm Nov 9, 2020

jordanschalm Nov 9, 2020

ramtinms Nov 9, 2020

jordanschalm Nov 9, 2020

ramtinms Nov 9, 2020

ramtinms Nov 9, 2020

jordanschalm Nov 9, 2020

ramtinms Nov 18, 2020

jordanschalm Nov 20, 2020

ramtinms commented Dec 2, 2020

	eventChunks := ChunkfiyEvents(events, chunkSize)
	eventChunks := ChunkifyEvents(events, chunkSize)

	func TestChunkfiyEvents(t *testing.T) {
	func TestChunkifyEvents(t *testing.T) {

Break batch event storage into smaller chunks #122

Break batch event storage into smaller chunks #122

Conversation

ramtinms commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramtinms commented Dec 2, 2020