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

refactor: Create meta.DB interface #92

Closed
wants to merge 3 commits into from

Conversation

twavv
Copy link
Contributor

@twavv twavv commented Apr 25, 2023

This is just the first PR in what will probably be a few. This one adds a basic database interface that just stores everything as a big map (which is probably fine for our scale of data) and writes to/reads from a JSON file as the persistence mechanism.

Wanted to open this now and get some feedback. In the next PR, I'll probably implement writing to refs while also writing to the JSON database. After that, the annoying change of having to plumb the database through everywhere.

@twavv twavv requested a review from draftcode April 25, 2023 22:43
@aviator-app
Copy link
Contributor

aviator-app bot commented Apr 25, 2023

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.

Stack

  1. 👉 refactor: Create meta.DB interface #92 👈 (this pr)

// WithTx runs the given function in a transaction. The transaction is
// committed when the function returns (unless it has been aborted by
// calling WriteTx.Abort).
WithTx(func(tx WriteTx)) error
Copy link
Contributor

Choose a reason for hiding this comment

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

WithTx(func(tx WriteTx) error)) error might be more convenient as we can use it in RunE directly.

RunE: func(...) error {
    db := ...
    return db.WriteTx(func(tx WriteTx) error) error {
        ...
    })
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was actually my original strategy but I chose against it to make it more clear that it's not a direct passthrough -- WriteTx can returns its own errors. Honestly, in hindsight, I think just doing a

tx := db.WriteTx()
tx.SetBranch(...)
tx.Commit()

will be a better API. I chose the function-arg version to make it impossible to forget to commit/abort a transaction, but I think it's fine to just have an API invariant of "you MUST call Commit or Abort on this transaction object".

@twavv twavv changed the title feat: Create meta.DB interface refactor: Create meta.DB interface Apr 27, 2023
Copy link
Contributor

@draftcode draftcode left a comment

Choose a reason for hiding this comment

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

Rereviewed for the updated interface.

// Abort aborts the transaction (no changes will be committed).
// The transaction cannot be used after it has been aborted.
Abort()
// Commit commits the transaction (all changes will be committed).
Copy link
Contributor

Choose a reason for hiding this comment

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

Small ask: Can we add a contract like "Abort can be called even after Commit". Otherwise, we need to cancel the Cleanup.

@aviator-app aviator-app bot added the blocked label May 3, 2023
@aviator-app
Copy link
Contributor

aviator-app bot commented May 3, 2023

This pull request failed to merge: top queued PR in the stack failed to merge. Remove the blocked label to re-queue.

@twavv twavv force-pushed the travis/mer-2205-create-av-cli-db-interface branch from 5718042 to 68f461b Compare May 3, 2023 17:36
@twavv twavv removed the blocked label May 3, 2023
@aviator-app aviator-app bot closed this May 3, 2023
@twavv twavv deleted the travis/mer-2205-create-av-cli-db-interface branch November 8, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants