feat: add support for using sqlite for storing delete requests #16437
Conversation
…r storing delete requests
…ing from an old db type to a newer one
sandeepsukhani
requested review from
trevorwhitney and
a team
as code owners
github-actions
bot
added
the
type/docs
|
💻 Deploy preview deleted. |
| } | ||
|
|
||
| s.connPool, err = sqlitex.NewPool(s.path, sqlitex.PoolOptions{}) | ||
| return err |
There was a problem hiding this comment.
It appears we are generally handling errors related to the sqlite db in a consistent manner, that is, returning err to the caller.
With that said, do we need to handle a database corruption in a more resilient manner? Or, is the answer to something like this "delete the database and restart the compactor"
There was a problem hiding this comment.
Yeah, the answer to database corruption is "delete the database and restart the compactor" because our code cannot handle it. The other option is to find a working version(assuming versioning is enabled on object storage) and restore it. Also, it would be a serious issue, so it warrants the operators' attention.
| processed_shards INT DEFAULT 0, | ||
| query TEXT NOT NULL | ||
| );` | ||
| sqlCreateDeleteRequestShardsTable = `CREATE TABLE IF NOT EXISTS shards ( |
There was a problem hiding this comment.
Perhaps we should also add some indicies?
CREATE INDEX idx_shards_id ON shards(id);
CREATE INDEX idx_shards_user_id ON shards(user_id);
CREATE INDEX idx_shards_time ON shards(start_time, end_time);
CREATE INDEX idx_requests_user_id ON requests(user_id);
There was a problem hiding this comment.
good idea. I have added indices considering the most frequently run queries.
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| func createDeleteRequestsStore(DeleteRequestsStoreDBType DeleteRequestsStoreDBType, workingDirectory string, indexStorageClient storage.Client, now func() model.Time) (DeleteRequestsStore, error) { |
There was a problem hiding this comment.
Is the purpose of passing a now() function here solely for the test case that uses it? It appears that generally this is just a wrapped Time.Now() call
There was a problem hiding this comment.
yeah, we use it to have a fixed creation time for delete requests to make it easier to compare them in tests.
Here is an example
There was a problem hiding this comment.
I just wonder if it would be cleaner to not have that parameter, and mock the store in the test case to have this behavior. I don't feel that adding a parameter that is only used by test cases is necessarily the cleanest.
What this PR does / why we need it:
Add support for using sqlite instead of boltdb for storing delete requests.
I have added a flag to choose between boltdb vs SQLite for storing the requests, which defaults to boltdb.
When sqlite is selected for storing the requests, the factory function will copy the data from boltdb to sqlite to populate the existing data.
I have also added support for using a backup store to reduce the stress of migration. It would use a tee to perform all the write operations on both the stores. Currently, the config only allows using boltdb as a backup store since users will be moving from boltdb to SQLite.
Checklist