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

Txn is too big to fit into one request when use disk store #3879

Closed
floriangasc opened this issue Oct 10, 2021 · 5 comments · Fixed by #3880
Closed

Txn is too big to fit into one request when use disk store #3879

floriangasc opened this issue Oct 10, 2021 · 5 comments · Fixed by #3880
Labels

Comments

@floriangasc
Copy link
Contributor

floriangasc commented Oct 10, 2021

Expected Behavior

  • no badger error «Txn is too big to fit into one request» when load bundle (or data.json)

Actual Behavior

opa process is abort due to badger error

Steps to Reproduce the Problem

Load a data with enough key and enough value by key for trigger error (see link in additionnal info part)

  • OPA version: 0.33.1
  • no need query, juste run opa. for me it seems to happen with a 30/40 mo data.json and contain around 400 key

another subject

*  pull bundles seems to take a lot of ram too, but problem can be solve with better data management (with solution like opal, for example)

  • probe seems to take ram also. It seems to have a difference of ram consumption between with and without probe ( /health)

Additional Info

  • i made a pull request with quick (??? and dirty ???) fix bug of «Txn is too big to fit into one request»
  • And in the same time i have unlock the storage disk for runtime (cli/server: cli param are added).
  • To solve the bug, i followed the get started page of badger doc, and applied it to storage/disk/txn.go file.
  • other useful link
  • Sorry for my english, and my go code (it's first time i write code in go).
  • I hope my work can help
@srenatus
Copy link
Contributor

This is great. 😃 Thanks a lot for both playing with the experimental stuff and fixing bugs in the process! 👏 🚀

As for the pull request, the contributions here are very welcome -- but let's take it one careful step at a time.

A good first step would be to reproduce the error condition in a test case, and add the fix to turn the test case green.

Let me know if you need any sort of assistance here!

@floriangasc
Copy link
Contributor Author

floriangasc commented Oct 11, 2021

Hi.

Thanks for your reply.

I started to write a test (and learned how). i have create storage/disk/txn_test.go with one method call "TestTxnIsTooBigToFitIntoOneRequestWhenUseDiskStore". (see below). Now i have a test that run with tiny json.
But i need 30 ou 40 mo of json. What is your prefered way for handle this data?

  1. a generate a json file and commit, load a json in test
  2. a generate a data on the fly inside test ?
  3. other ?

Thanks in advance for your rely.

func TestTxnIsTooBigToFitIntoOneRequestWhenUseDiskStore(t *testing.T) {
	test.WithTempFS(map[string]string{}, func(dir string) {
		ctx := context.Background()
		s, err := disk.New(ctx, disk.Options{Dir: dir, Partitions: []storage.Path{
			storage.MustParsePath("/foo/bar"),
		}})
		if err != nil {
			t.Fatal(err)
		}

		errTxn := storage.Txn(ctx, s, storage.WriteParams, func(txn storage.Transaction) error {
			jsonFixture := util.MustUnmarshalJSON([]byte(`{"foo":{"bar":"a"}}`))

			errTxnWrite := s.Write(ctx, txn, storage.AddOp, storage.MustParsePath("/foo/bar"), jsonFixture)

			if errTxnWrite != nil {
				t.Fatal(errTxnWrite)
			}
			return nil
		})

		if errTxn != nil {
			t.Fatal(errTxn)
		}
	})
}

@srenatus
Copy link
Contributor

@floriangasc thanks for bearing with me! I think both would be OK here; I'd slightly prefer (2.) because we wouldn't have to carry gigantic json blobs in git then.... We've already got a few helpers for generating data, I suppose, perhaps something there could be re-used? (Like

func GenerateJSONBenchmarkData(k, v int) map[string]interface{} {
)

@floriangasc
Copy link
Contributor Author

awesome i look that just after lunch.

@floriangasc
Copy link
Contributor Author

floriangasc commented Oct 11, 2021

Hi,

Sorry for late, i am not full time on this but i spend time as most as possible (multiple time by day).

I pushed a new version little-work-on-disk-storage with the new file storage/disk/txn_test.go that contains a test which not pass on main branch (with the «txn_test.go:149: storage_internal_error: Txn is too big to fit into one request») and pass on this branch

If you have any advice on my code, do not hesitate :)

Ps : I Wonder if test should assert no key is lost

srenatus pushed a commit that referenced this issue Oct 20, 2021
Fixes ##3879.

Signed-off-by: Gasc Florian <florian.gasc@gmail.com>
dolevf pushed a commit to dolevf/opa that referenced this issue Nov 4, 2021
Fixes #open-policy-agent#3879.

Signed-off-by: Gasc Florian <florian.gasc@gmail.com>
Signed-off-by: Dolev Farhi <farhi.dolev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants