storage/disk: try fix for bader error «txn is too big to fit into one… #3880

floriangasc · 2021-10-10T17:31:33Z

… request»

runtime: unlock choice before disk and in memory storage

Signed-off-by: Gasc Florian florian.gasc@gmail.com
Fixes #3879

srenatus

Thanks a lot again, this is great. There are a few things to take care of, but let's fix this bug 💯

storage/disk/txn_test.go

storage/disk/txn.go

cmd/run.go

floriangasc · 2021-10-13T13:16:01Z

Hi,

Tanks for your review. I made correction.

use helper for json
clean code on fixed code
(re) hide disk from cli: i restored run.go and runtime.go from main.

ps:

i don't really know/understand the whole process for P.R and work on this kind of project. It's only the second time i contribute to something. (the first is ng2 a long time ago). Please excuse me in advance if i do something wrong.
i don't know if you saw the comment i wrote on issue about : if unit (non regression) test should assert no key are lost. I wonder if it's badger or OPA reponsability to check that when this error occured.

srenatus

Just some nitpicks, let's get this merged and take it from there 👍 Thanks again for looking into this and bearing with me.

storage/disk/txn.go

storage/disk/txn_test.go

storage/disk/txn.go

floriangasc · 2021-10-14T09:52:20Z

hi

Thanks for pertinent feedback.

i made change, but there is one question in suspend (writed in PS.2 of my previous comment) and i would be sure about what should be test:

unit (non regression) test should assert no key are lost ? ( I wonder if it's badger or OPA reponsability to check that when this error occured.)

srenatus · 2021-10-14T09:54:39Z

unit (non regression) test should assert no key are lost ? ( I wonder if it's badger or OPA reponsability to check that when this error occured.)

It think it would make sense to have some read-back to assert what's been stored under this condition. After all, it's not obvious to me: if the txn was too big, and we commit it, did it discard stuff or still work...?

That aside, thanks for bearing with me on all the comments 👍

floriangasc · 2021-10-14T11:03:33Z

i think too. it's better to assert the state is ok by read what is inserted.

if it's ok for you, i should add this test for assert no key are lost and i repush the test with new assert.

I await your a approvement for doing this assert :)

srenatus · 2021-10-14T11:19:38Z

@floriangasc thank you! sounds like a good way forward.

floriangasc · 2021-10-14T13:18:59Z

the test of read catch a very not cool bug.

I will come back to you when i will have resolved the bug: some keys are lost, and there is a problem with re-assign the txn in the disk store. I study the code now.

May be there will be some question.

srenatus · 2021-10-14T16:08:23Z

@floriangasc I've converted this to draft, feel free to click "ready for review" when it's time!

floriangasc · 2021-10-14T17:19:33Z

Hi.

i have done with test «no lost key».

The explanation of the previous message:

the first problem is when i change the code according p.r i introduce a bug (I should have run the tests before commit/push :( )
The second problem occured when i generate key. because of generate lot of (140_000) key with 3 random latter, when the array is converted into hashmap (json object) the same key are collapse, so i get less than 140_000 ( with number of missing key is random). But before I release that, for debug, i have introduce a «false bug» with commit transaction 2 times.
In the same time, i have the made a tiny rewrite of my code. Let me see's if you think it's better

At the end

the initial "txn is too big to fit into one request", is fixed
the problem introduce by the P.R are fixed
there a test that insert 140_000 keys with each have value (of 3 characters) that is enough for trigger the bug and test when error occurred, when we read the store after that, the 140_000 key are present

floriangasc · 2021-10-15T09:59:39Z

hi,

For the P.R there are some points which must be treated with attention

I don't find a better way to (set|delete)WithErrTxnTooBigErrorHandling function. If you have better idea, you are welcome
I don't know how to solve circular import in test: "github.com/open-policy-agent/opa/storage/disk"

storage/disk/txn_test.go

floriangasc · 2021-10-15T15:58:19Z

What is the next step ?

srenatus · 2021-10-15T16:22:15Z

We'll get this reviewed and merged 😄 can you also please squash the commits? One should be fine.

srenatus · 2021-10-15T16:26:11Z

Also getting the tests green would be great.

floriangasc · 2021-10-15T16:35:42Z

Great. i doing this monday!

srenatus · 2021-10-18T12:47:38Z

storage/disk/txn_test.go

+		}
+		actualNbKeys := len(result.(map[string]interface{}))
+		if nbKeys != actualNbKeys {
+			t.Fatalf("Unexpected <~nbKeys: " + strconv.Itoa(nbKeys) + " ~> read whereas <~actualNbKeys: " + strconv.Itoa(actualNbKeys) + "~> inserted")


[nit]

Suggested change

t.Fatalf("Unexpected <~nbKeys: " + strconv.Itoa(nbKeys) + " ~> read whereas <~actualNbKeys: " + strconv.Itoa(actualNbKeys) + "~> inserted")

t.Fatalf("Expected %d keys, read %d", nbKeys, actualNbKeys)

Good, fixed.

floriangasc · 2021-10-18T13:06:55Z

All checks have passed \o/\o/ :)

srenatus

This is getting close, thanks for bearing with me. One more question re: the delete case, and a small nitpick. I think this is the last round 🤞

storage/disk/txn.go

… non regression test Signed-off-by: Gasc Florian <florian.gasc@gmail.com>

floriangasc · 2021-10-20T16:49:39Z

Hi, @srenatus

Just for be sure if i have done a good job and all it's ok ?

srenatus

LGTM. Thanks again!

Fixes #open-policy-agent#3879. Signed-off-by: Gasc Florian <florian.gasc@gmail.com> Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>

floriangasc force-pushed the little-work-on-disk-storage branch from 3b871b1 to fc23131 Compare October 11, 2021 05:05

srenatus reviewed Oct 11, 2021

View reviewed changes

storage/disk/txn_test.go Outdated Show resolved Hide resolved

storage/disk/txn.go Outdated Show resolved Hide resolved

storage/disk/txn.go Outdated Show resolved Hide resolved

storage/disk/txn.go Outdated Show resolved Hide resolved

cmd/run.go Outdated Show resolved Hide resolved

floriangasc force-pushed the little-work-on-disk-storage branch from eb3dd98 to 94490c0 Compare October 13, 2021 13:14

floriangasc force-pushed the little-work-on-disk-storage branch from 94490c0 to 25629e2 Compare October 13, 2021 13:54

srenatus reviewed Oct 14, 2021

View reviewed changes

storage/disk/txn.go Outdated Show resolved Hide resolved

storage/disk/txn_test.go Outdated Show resolved Hide resolved

storage/disk/txn.go Outdated Show resolved Hide resolved

storage/disk/txn.go Outdated Show resolved Hide resolved

storage/disk/txn.go Outdated Show resolved Hide resolved

srenatus marked this pull request as draft October 14, 2021 16:07

floriangasc force-pushed the little-work-on-disk-storage branch 2 times, most recently from d9b1eb4 to fa86ba8 Compare October 14, 2021 17:08

floriangasc marked this pull request as ready for review October 15, 2021 09:05

srenatus reviewed Oct 15, 2021

View reviewed changes

storage/disk/txn_test.go Outdated Show resolved Hide resolved

floriangasc force-pushed the little-work-on-disk-storage branch 5 times, most recently from 905be3a to b8e8836 Compare October 18, 2021 12:45

srenatus reviewed Oct 18, 2021

View reviewed changes

floriangasc force-pushed the little-work-on-disk-storage branch from b8e8836 to 3c48577 Compare October 18, 2021 12:49

floriangasc force-pushed the little-work-on-disk-storage branch 2 times, most recently from bfe08b4 to a74e74b Compare October 18, 2021 12:58

srenatus reviewed Oct 19, 2021

View reviewed changes

storage/disk/txn.go Show resolved Hide resolved

storage/disk/txn.go Outdated Show resolved Hide resolved

floriangasc force-pushed the little-work-on-disk-storage branch 5 times, most recently from 4985d46 to 05efce2 Compare October 20, 2021 05:29

storage/dik/txn.go: fix «txn is too big to fit into one request» with…

54787cf

… non regression test Signed-off-by: Gasc Florian <florian.gasc@gmail.com>

floriangasc force-pushed the little-work-on-disk-storage branch from 05efce2 to 54787cf Compare October 20, 2021 05:30

Merge branch 'main' into little-work-on-disk-storage

a081e90

srenatus approved these changes Oct 20, 2021

View reviewed changes

srenatus merged commit 528836a into open-policy-agent:main Oct 20, 2021

floriangasc deleted the little-work-on-disk-storage branch October 21, 2021 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage/disk: try fix for bader error «txn is too big to fit into one… #3880

storage/disk: try fix for bader error «txn is too big to fit into one… #3880

floriangasc commented Oct 10, 2021

srenatus left a comment

floriangasc commented Oct 13, 2021 •

edited

Loading

srenatus left a comment

floriangasc commented Oct 14, 2021 •

edited

Loading

srenatus commented Oct 14, 2021

floriangasc commented Oct 14, 2021 •

edited

Loading

srenatus commented Oct 14, 2021

floriangasc commented Oct 14, 2021 •

edited

Loading

srenatus commented Oct 14, 2021

floriangasc commented Oct 14, 2021 •

edited

Loading

floriangasc commented Oct 15, 2021

floriangasc commented Oct 15, 2021

srenatus commented Oct 15, 2021

srenatus commented Oct 15, 2021

floriangasc commented Oct 15, 2021

srenatus Oct 18, 2021

floriangasc Oct 18, 2021

floriangasc commented Oct 18, 2021 •

edited

Loading

srenatus left a comment

floriangasc commented Oct 20, 2021

srenatus left a comment

	t.Fatalf("Unexpected <~nbKeys: " + strconv.Itoa(nbKeys) + " ~> read whereas <~actualNbKeys: " + strconv.Itoa(actualNbKeys) + "~> inserted")
	t.Fatalf("Expected %d keys, read %d", nbKeys, actualNbKeys)

storage/disk: try fix for bader error «txn is too big to fit into one… #3880

storage/disk: try fix for bader error «txn is too big to fit into one… #3880

Conversation

floriangasc commented Oct 10, 2021

srenatus left a comment

Choose a reason for hiding this comment

floriangasc commented Oct 13, 2021 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

floriangasc commented Oct 14, 2021 • edited Loading

srenatus commented Oct 14, 2021

floriangasc commented Oct 14, 2021 • edited Loading

srenatus commented Oct 14, 2021

floriangasc commented Oct 14, 2021 • edited Loading

srenatus commented Oct 14, 2021

floriangasc commented Oct 14, 2021 • edited Loading

floriangasc commented Oct 15, 2021

floriangasc commented Oct 15, 2021

srenatus commented Oct 15, 2021

srenatus commented Oct 15, 2021

floriangasc commented Oct 15, 2021

srenatus Oct 18, 2021

Choose a reason for hiding this comment

floriangasc Oct 18, 2021

Choose a reason for hiding this comment

floriangasc commented Oct 18, 2021 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

floriangasc commented Oct 20, 2021

srenatus left a comment

Choose a reason for hiding this comment

floriangasc commented Oct 13, 2021 •

edited

Loading

floriangasc commented Oct 14, 2021 •

edited

Loading

floriangasc commented Oct 14, 2021 •

edited

Loading

floriangasc commented Oct 14, 2021 •

edited

Loading

floriangasc commented Oct 14, 2021 •

edited

Loading

floriangasc commented Oct 18, 2021 •

edited

Loading