test-only: In-Memory UC-Commits-Client #1644
Conversation
There was a problem hiding this comment.
Rename from commits_client.rs to mod.rs. Glad GitHub captures this succinctly!
| mod in_memory; | ||
|
|
||
| #[cfg(any(test, feature = "test-utils"))] | ||
| pub use in_memory::InMemoryCommitsClient; |
There was a problem hiding this comment.
The InMemory UC-Commits-Client is only visible when (a) inside tests, and (b) using the test-utils feature flag.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1644 +/- ##
==========================================
+ Coverage 84.16% 84.28% +0.12%
==========================================
Files 122 124 +2
Lines 34111 34423 +312
Branches 34111 34423 +312
==========================================
+ Hits 28710 29015 +305
+ Misses 4013 3993 -20
- Partials 1388 1415 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// The highest version that has been ratified (committed) to this table. | ||
| max_ratified_version: i64, |
There was a problem hiding this comment.
Just for my knowledge: do we use the word "ratified" as a specific state in design docs for catalog-based commits?
There was a problem hiding this comment.
Not just design docs, but also the table feature protocol specification, too. https://github.com/delta-io/delta/blob/master/protocol_rfcs/catalog-managed.md#terminology-commits
Writers propose commits to the catalog, which can either reject the commit, or approve (aka "ratify") it.
| pub fn create_table(&self, table_id: impl Into<String>) -> Result<()> { | ||
| let mut tables = self.tables.write().unwrap(); | ||
| match tables.entry(table_id.into()) { | ||
| std::collections::hash_map::Entry::Vacant(e) => { |
There was a problem hiding this comment.
maybe use std::collections::hash_map; above?
| std::collections::hash_map::Entry::Vacant(e) => { | |
| hash_map::Entry::Vacant(e) => { |
| ))); | ||
| } | ||
|
|
||
| if self.catalog_commits.len() as u16 >= Self::MAX_UNPUBLISHED_COMMITS { |
There was a problem hiding this comment.
why the u16 instead of usize?
|
|
||
| impl UCCommitsClient for InMemoryCommitsClient { | ||
| async fn get_commits(&self, request: CommitsRequest) -> Result<CommitsResponse> { | ||
| let tables = self.tables.read().unwrap(); |
There was a problem hiding this comment.
Do we expect external folks to use this test? We should probably make this an error. Maybe a Fatal variant?
NVM, seems this is test-only.
3 participants
🥞 Stacked PR
Use this link to review incremental changes.
What changes are proposed in this pull request?
This PR adds a fairly simple In-Memory UC-Commits-Client for tests.
We will then be able to build e2e read/write/publish tests on top of this client, later.
How was this change tested?
Add some simple tests for this client.