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

Use new value cache for json.match_schema #7081

Merged

Conversation

anderseknert
Copy link
Member

I figured I'd test this out anyway, and this seemed like a good case given that there was an actual issue on this.

Testing response times with OPA running as a server, and the first request is ~800 ms while the following ones are ~10 ms.

Fixes #7011

Why the changes in this PR are needed?

What are the changes in this PR?

Notes to assist PR review:

Further comments:

charlieegan3
charlieegan3 previously approved these changes Oct 1, 2024
},
)
if err != nil {
t.Fatalf("Unexpected schema validation errorerror: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Fatalf("Unexpected schema validation errorerror: %s", err)
t.Fatalf("Unexpected schema validation error: %s", err)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

srenatus
srenatus previously approved these changes Oct 1, 2024
Copy link
Contributor

@srenatus srenatus 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 this warrants some docs addition -- now, something else is going to live in the cache, too, and that's relevant for reasoning about a desired value of max_entries.

@@ -5,6 +5,8 @@
package topdown

import (
"context"
"github.com/open-policy-agent/opa/topdown/cache"
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] import ordering seems off here

@anderseknert anderseknert dismissed stale reviews from srenatus and charlieegan3 via 7bf54ba October 1, 2024 12:31
@anderseknert anderseknert force-pushed the jsonschema-match-cache branch from f0f9611 to 7bf54ba Compare October 1, 2024 12:31
Copy link

netlify bot commented Oct 1, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 7bf54ba
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/66fbeb95aec24c0008125cad
😎 Deploy Preview https://deploy-preview-7081--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@anderseknert
Copy link
Member Author

Added docs @srenatus 👍

I figured I'd test this out anyway, and this seemed like
a good case given that there was an actual issue on this.

Testing response times with OPA running as a server, and
the first request is ~800 ms while the following ones are
~10 ms.

Fixes open-policy-agent#7011

Signed-off-by: Anders Eknert <anders@styra.com>
@anderseknert anderseknert force-pushed the jsonschema-match-cache branch from 7bf54ba to 291a3c2 Compare October 1, 2024 12:33
@anderseknert anderseknert merged commit 6303aa2 into open-policy-agent:main Oct 1, 2024
28 checks passed
@anderseknert anderseknert deleted the jsonschema-match-cache branch October 1, 2024 12:46
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.

json.match_schema performance
3 participants