Conversation
lib/shard/src/wal.rs
Outdated
| /// (this is used to extend recoverable history and allow WAL shard transfers) | ||
| const INCREASED_RETENTION_FACTOR: usize = 10; | ||
|
|
||
| pub struct WalSerializedRecord<R> { |
There was a problem hiding this comment.
Nit: I'd like WalRawRecord a bit better
| impl<R: DeserializeOwned + Serialize> WalSerializedRecord<R> { | ||
| pub fn new(record: &R) -> Result<Self> { | ||
| // ToDo: Replace back to faster rmp, once this https://github.com/serde-rs/serde/issues/2055 solved | ||
| let record = serde_cbor::to_vec(record).map_err(|err| { |
There was a problem hiding this comment.
This vec seems to over allocate (by powers of two). But we only keep these in memory for a short period, don't we?
|
It makes using the WAL slightly more difficult, but I understand the benefit. Not locking the WAL during (de)serialization can be significant with very large operations. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/shard/src/wal.rs`:
- Around line 40-46: The error message in Wal::new (the new function) is
misleading because serde_cbor::to_vec failing is a serialization error before
any WAL I/O; update the WalError::WriteWalError constructed in the map_err for
serde_cbor::to_vec to state that serialization of the provided record (type R)
failed — e.g., "Failed to serialize record for WAL (Serialize impl error):
{err:?}" — and keep including the original err details to aid debugging; locate
the serde_cbor::to_vec(...) map_err in pub fn new(record: &R) -> Result<Self>
and replace the misleading "corrupted WAL or version mismatch" text accordingly.
🧹 Nitpick comments (2)
lib/shard/src/wal.rs (2)
34-37:WalRawRecordlacks common trait derives.Consider deriving
Debug(and possiblyClone) forWalRawRecord<R>to aid diagnostics and flexibility. WithoutDebug, it can't be used in debug-format assertions or error messages.Proposed change
+#[derive(Debug, Clone)] pub struct WalRawRecord<R> { record: Vec<u8>, _phantom: PhantomData<R>, }
53-58: Redundant trait bound ondeserialize.The
where R: DeserializeOwnedclause is already specified on theimplblock (line 39). The additional bound on this method is unnecessary.Proposed fix
- pub fn deserialize(&self) -> Result<R> - where - R: DeserializeOwned, - { + pub fn deserialize(&self) -> Result<R> { Self::deserialize_from(&self.record) }Same applies to
deserialize_fromat line 60-62.
| pub fn new(record: &R) -> Result<Self> { | ||
| // ToDo: Replace back to faster rmp, once this https://github.com/serde-rs/serde/issues/2055 solved | ||
| let record = serde_cbor::to_vec(record).map_err(|err| { | ||
| WalError::WriteWalError(format!( | ||
| "Can't serialize entry, probably corrupted WAL or version mismatch: {err:?}" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
Misleading error message on the serialization (write) path.
The error says "corrupted WAL or version mismatch" but this is the serialization path — the data hasn't touched the WAL yet. A serialization failure here is more likely a bug in the operation type's Serialize impl, not a corrupted WAL.
Suggested fix
let record = serde_cbor::to_vec(record).map_err(|err| {
WalError::WriteWalError(format!(
- "Can't serialize entry, probably corrupted WAL or version mismatch: {err:?}"
+ "Can't serialize WAL entry: {err:?}"
))
})?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn new(record: &R) -> Result<Self> { | |
| // ToDo: Replace back to faster rmp, once this https://github.com/serde-rs/serde/issues/2055 solved | |
| let record = serde_cbor::to_vec(record).map_err(|err| { | |
| WalError::WriteWalError(format!( | |
| "Can't serialize entry, probably corrupted WAL or version mismatch: {err:?}" | |
| )) | |
| })?; | |
| pub fn new(record: &R) -> Result<Self> { | |
| // ToDo: Replace back to faster rmp, once this https://github.com/serde-rs/serde/issues/2055 solved | |
| let record = serde_cbor::to_vec(record).map_err(|err| { | |
| WalError::WriteWalError(format!( | |
| "Can't serialize WAL entry: {err:?}" | |
| )) | |
| })?; |
🤖 Prompt for AI Agents
In `@lib/shard/src/wal.rs` around lines 40 - 46, The error message in Wal::new
(the new function) is misleading because serde_cbor::to_vec failing is a
serialization error before any WAL I/O; update the WalError::WriteWalError
constructed in the map_err for serde_cbor::to_vec to state that serialization of
the provided record (type R) failed — e.g., "Failed to serialize record for WAL
(Serialize impl error): {err:?}" — and keep including the original err details
to aid debugging; locate the serde_cbor::to_vec(...) map_err in pub fn
new(record: &R) -> Result<Self> and replace the misleading "corrupted WAL or
version mismatch" text accordingly.
|
@IvanPleshkov there still seems to be a merge conflict. |
d2aaaac to
a3b7490
Compare
* dont lock WAL while serialization * review remarks * remove error log, its already logged in update worker
This PR is a try to fix an empty update queue issue, where the upate queue is empty under pressure on some configurations.
Local tests didnt show differences but it seems this changes are nice to merge anyway.
The core idea is making WAL lock more efficient. Currently, we lock WAL for IO and for serialization.
Actually, it's not needed to lock the WAL for expensive serialization; this PR fixes it by introducing the serializable record entity,