refactor: reduce catalog locks when getting chunks #25896
Merged
+269
−265
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The main refactor was to change the ChunkContainer trait to use the DatabaseSchema and TableDefinition types directly in the signature, vs. the names, which then required an additional catalog lock and lookups for both entities. This was already handled upstream in the QueryTable, so there was no need to do the lookups again. This required the addition of a test helper in influxdb3_write::test_helpers that provides convenience methods for getting record batches from the WriteBuffer. We have been implementing such a method manually in several places, so this is nice to have it unified. This provides a blanket impl so that anything implementing WriteBuffer gets the method. Some other house cleaning was included.
hiltontj
force-pushed
the
hiltontj/simplify-query-buffer-traits
branch
from
4cc402a to
57d5ab9
Compare
praveen-influx
approved these changes
Jan 22, 2025
Yep, that is correct. I am referring to the read lock when calling I am going to hold off on merging this for a bit as I reason through some downstream changes needed in enterprise (see https://github.com/influxdata/influxdb_pro/issues/451#issuecomment-2607704757). I need to make sure this doesn't break things for that work. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main refactor was to change the
ChunkContainertrait to use theDatabaseSchemaandTableDefinitiontypes directly in the signature, vs. their names; using their names led to an additional catalog lock and lookups for both entities, because there was already a catalog lock/lookup upstream in theQueryTable.However, using the names in the
ChunkContainer::get_table_chunkswas convenient, specifically for tests, so I included a test helper trait ininfluxdb3_write::test_helperscalledWriteBufferTesterthat provides convenience methods for getting record batches from aWriteBufferimplementation. Alongside the definition ofWriteBufferTesteris a blanketimplon that anyTthat implementsWriteBuffer.We have been implementing similar methods manually in several places when testing in Enterprise, so this should give us a unified implementation for that.
Some other house cleaning was included 🧹
influxdb3_wal::TableDefinitiontoWalTableDefinitionto reduce naming conflicts.BufferFiltertoChunkFilter, and within that,BufferGuaranteewas changed toHashedLiteralGuaranteetable_buffercode