Skip to content

feat: DELETE last cache API#25162

Merged
hiltontj merged 64 commits intomainfrom
hiltontj/lastcache-delete-api
Jul 16, 2024
Merged

feat: DELETE last cache API#25162
hiltontj merged 64 commits intomainfrom
hiltontj/lastcache-delete-api

Conversation

@hiltontj
Copy link
Copy Markdown
Contributor

Closes #25097

Adds an API for deleting last caches. See the issue for details about the API. Some notable aspects of the change:

  • The API allows parameters to be passed in either the request URI query string, or in the body as JSON
  • Some additional error modes were handled, specifically, for better HTTP status code responses, e.g., invalid content type is now a 415, URL query string parsing errors are now 400
  • An end-to-end test was added to check behaviour of the API

hiltontj added 30 commits June 27, 2024 12:01
Each last cache holds a ring buffer for each column in an index map, which
preserves the insertion order for faster record batch production.

The ring buffer uses a custom type to handle the different supported
data types that we can have in the system.
LastCacheProvider is the API used to create last caches and write
table batches to them. It uses a two-layer RwLock/HashMap: the first for
the database, and the second layer for the table within the database.

This allows for table-level locks when writing in buffered data, and only
gets a database-level lock when creating a cache (and in future, when
removing them as well).
Added basic APIs on the write buffer to access the last cache and then a
test to the last_cache module to see that it works with a simple example
Addressed three parts of PR feedback:

1. Remove double-lock on cache map
2. Re-order the get when writing to the cache to be outside the loop
3. Move the time check into the cache itself
This refactors the last cache to use a nested caching structure, where
the key columns for a given cache are used to create a hierarchy of
nested maps, terminating in the actual store for the values in the cache.

Access to the cache is done via a set of predicates which can optionally
specify the key column values at any level in the cache hierarchy to only
gather record batches from children of that node in the cache.

Some todos:
- Need to handle the TTL
- Need to move the TableProvider impl up to the LastCache type
This re-writes the datafusion TableProvider implementation on the correct
type, i.e., the LastCache, and adds conversion from the filter Expr's to
the Predicate type for the cache.
Last caches will have expired entries walked when writes come in.
Changed key columns so that they do not accept null values, i.e., rows
that are pushed that are missing key column values will be ignored.

When producing record batches for a cache, if not all key columns are
used in the predicate, then this change makes it so that the non-predicate
key columns are produced as columns in the outputted record batches.

A test with a few cases showing this was added.
Ensure key columns in the last cache that are not included in the
predicate are emitted in the RecordBatches as a column.

Cleaned up and added comments to the new test.
Added two tests, as per commit title. Also moved the eviction process
to a separate function so that it was not being done on every write to
the cache, which could be expensive, and this ensures that entries are
evicted regardless of whether writes are coming in or not.
CacheAlreadyExists errors were only being based on the database and
table names, and not including the cache names, which was not
correct.
This also adds explicit support for series key columns to distinguish
them from normal tags in terms of nullability

A test was added to check nulls work
Support the addition of new fields to the last cache, for caches that do
not have a specified set of value columns.

A test was added along with the changes.
hiltontj added 13 commits July 11, 2024 13:50
The server used to accept a socket address and bind it directly, returning
error if the bind fails.

This commit changes that so the ServerBuilder accepts a TcpListener. The
behaviour is essentially the same, but this allows us to bind the address
from tests when instantiating the server, so we can easily assign unused
ports.

Tests in the influxdb3_server were updated to exploit this in order to
use port 0 auto assignment and stop flaky test failures.

A new, failing, test was also added to that module for the last cache.
Committing here as the last cache is in a working state, but it is naively
implemented as it just stores all key columns again (still with the hierarchy)
I also moved the write lock in the create_cache function for the
LastCacheProvider, to prevent a race condition between the time it was
getting the read lock to check the existence of the cache, and when it
gets the write lock to insert the new cache. Now, it just uses a single
write lock to do the check, and the subsequent insert.
This adds a method on the LastCacheProvider to delete a cache, and clean
up the hierarchy for empty hashmaps.

A new HTTP API was added to delete caches, and a test was added to test
the API in an E2E test case.
@hiltontj hiltontj added the v3 label Jul 15, 2024
@hiltontj hiltontj requested a review from pauldix July 15, 2024 20:01
@hiltontj hiltontj self-assigned this Jul 15, 2024
@hiltontj hiltontj requested a review from mgattozzi July 15, 2024 20:03
Base automatically changed from hiltontj/lastcache-create-api to main July 16, 2024 14:32
@hiltontj hiltontj merged commit e8d9b02 into main Jul 16, 2024
@hiltontj hiltontj deleted the hiltontj/lastcache-delete-api branch July 16, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API for deletion of last_caches

2 participants