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

Add API support for transaction metadata on WriteRels and DeleteRels #2084

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

josephschorr
Copy link
Member

No description provided.

@josephschorr josephschorr requested a review from a team as a code owner October 1, 2024 21:07
@github-actions github-actions bot added area/api v1 Affects the v1 API area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Oct 1, 2024
tstirrat15
tstirrat15 previously approved these changes Oct 1, 2024
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

See comments, otherwise LGTM

@@ -42,6 +43,8 @@ var writeUpdateCounter = promauto.NewHistogramVec(prometheus.HistogramOpts{
Buckets: []float64{0, 1, 2, 5, 10, 15, 25, 50, 100, 250, 500, 1000},
}, []string{"kind"})

const MaximumTransactionMetadataSize = 65000 // bytes. Limited by the BLOB size used in MySQL driver
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth making this datastore-specific? Or will we burn that bridge when we get to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather set it to the low amount and reevaluate in the future if need be. "65K should be enough for anyone!"

}

if len(b) > MaximumTransactionMetadataSize {
return NewTransactionMetadataTooLargeErr(len(b), MaximumTransactionMetadataSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we guaranteed that the client encoding will match the JSON encoding? i.e. will the size that that the client sees on their machine match what we're validating against here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No; this is a rough estimate but since the drivers store JSON too, it should be sufficient

tstirrat15
tstirrat15 previously approved these changes Oct 1, 2024
@josephschorr josephschorr added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2024
@josephschorr josephschorr added this pull request to the merge queue Oct 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 4, 2024
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

Reapproving after rebase

@josephschorr josephschorr added this pull request to the merge queue Oct 4, 2024
Merged via the queue into authzed:main with commit 99a119e Oct 4, 2024
22 checks passed
@josephschorr josephschorr deleted the write-delete-metadata branch October 4, 2024 21:15
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/api v1 Affects the v1 API area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants