Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

write generated export padding to the database, ensure overlapping ex… #1119

Merged
merged 4 commits into from
Oct 29, 2020

Conversation

mikehelmick
Copy link
Contributor

…ports contain same data

Proposed Changes

  • Ensures that exports with overlapping time periods contain the same generated data by persisting the data to the database
  • Obtain a lock over export regions when creating exports, all region locks are required, and they are sorted and obtained in order
  • Batches are attempted to be processed in an order such that padding for earlier batches should be generated and thus included in later batches

Release Note

Export padding is persisted to the database to ensure stability over time.

@google-oss-robot google-oss-robot added the approved Auto: added by prow when enough reviewers approve. label Oct 29, 2020
@google-cla google-cla bot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Oct 29, 2020
@google-oss-robot google-oss-robot added the size/XL Auto: extra large number of changes. label Oct 29, 2020
@mikehelmick
Copy link
Contributor Author

ready to review but putting a
/hold
while I check on some things

@google-oss-robot google-oss-robot added the do-not-merge/hold Do not merge: held by author or reviewer. label Oct 29, 2020
internal/export/worker.go Outdated Show resolved Hide resolved
// MultiLock obtains multiple locks in a single transaction. Either all locks are obtained, or
// the transaction is rolled back.
// The lockIDs are sorted by normal ascending string sort order before obtaining the locks.
func (db *DB) MultiLock(ctx context.Context, lockIDs []string, ttl time.Duration) (UnlockFn, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this so we can lock across multiple tables? It's unclear to me why we don't just use a transaction instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comments to worker.go where we take the locks

@mikehelmick
Copy link
Contributor Author

/cancel hold

@mikehelmick
Copy link
Contributor Author

/unhold

@google-oss-robot google-oss-robot removed the do-not-merge/hold Do not merge: held by author or reviewer. label Oct 29, 2020
Copy link
Member

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

in case you have some other tweaks

@google-oss-robot google-oss-robot added the do-not-merge/hold Do not merge: held by author or reviewer. label Oct 29, 2020
@google-oss-robot google-oss-robot added the lgtm Auto: added by prown with a reviewer LGTMs label Oct 29, 2020
@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikehelmick, sethvargo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [mikehelmick,sethvargo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mikehelmick
Copy link
Contributor Author

/hold cancel

nope - this is good to go

@google-oss-robot google-oss-robot removed the do-not-merge/hold Do not merge: held by author or reviewer. label Oct 29, 2020
@google-oss-robot google-oss-robot merged commit 3a3417b into google:main Oct 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2020
@mikehelmick mikehelmick deleted the exgen branch November 6, 2020 22:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Auto: added by prow when enough reviewers approve. cla: yes Auto: added by CLA bot when all committers have signed a CLA. lgtm Auto: added by prown with a reviewer LGTMs size/XL Auto: extra large number of changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants