[PR #119] Abstract KV layer options to eliminate dynamic casts to FDBTransaction #297

New issue

Open

opened 2026-10-26 09:48:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 09:48:23 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/deepseek-ai/3FS/pull/119
Author: @Clcanny
Created: 3/6/2025
Status: 🔄 Open

Base: main ← Head: remove-dynamic-cast

📝 Commits (1)

47a3ed4 Abstract KV layer options to eliminate dynamic casts to FDBTransaction

📊 Changes

7 files changed (+51 additions, -9 deletions)

View changed files

📝 src/common/kv/ITransaction.h (+11 -0)
📝 src/common/kv/mem/MemTransaction.h (+7 -0)
📝 src/fdb/FDBTransaction.cc (+16 -0)
📝 src/fdb/FDBTransaction.h (+2 -0)
📝 src/meta/components/GcManager.cc (+4 -6)
📝 src/meta/store/Operation.h (+2 -3)
📝 tests/common/TestWithTransaction.cc (+9 -0)

📄 Description

The use of dynamic_cast can bypass the common functionality defined by the interface classes (IReadOnlyTransaction and IReadWriteTransaction) and instead rely on specific implementation details. This might not be ideal, as it reduces abstraction and increases coupling to a particular implementation. To address this, I attempted to define common option-setting functions.

I considered two approaches:

Providing a universal function such as setOption(TransactionOption option, std::string_view value) that can handle all options.
Using separate, specific functions like enableStaleRead and setPriority.

I chose the second approach because it assumes there are not many options to set, and this method is clearer and type-safe compared to passing raw value strings.

Thank you for taking the time to review. Feel free to close the MR if it is too small or not helpful.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/deepseek-ai/3FS/pull/119 **Author:** [@Clcanny](https://github.com/Clcanny) **Created:** 3/6/2025 **Status:** 🔄 Open **Base:** `main` ← **Head:** `remove-dynamic-cast` --- ### 📝 Commits (1) - [`47a3ed4`](https://github.com/deepseek-ai/3FS/commit/47a3ed4d744f1d9c1a6228206a09359adca3ea0d) Abstract KV layer options to eliminate dynamic casts to FDBTransaction ### 📊 Changes **7 files changed** (+51 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `src/common/kv/ITransaction.h` (+11 -0) 📝 `src/common/kv/mem/MemTransaction.h` (+7 -0) 📝 `src/fdb/FDBTransaction.cc` (+16 -0) 📝 `src/fdb/FDBTransaction.h` (+2 -0) 📝 `src/meta/components/GcManager.cc` (+4 -6) 📝 `src/meta/store/Operation.h` (+2 -3) 📝 `tests/common/TestWithTransaction.cc` (+9 -0) </details> ### 📄 Description The use of `dynamic_cast` can bypass the common functionality defined by the interface classes (`IReadOnlyTransaction` and `IReadWriteTransaction`) and instead rely on specific implementation details. This might not be ideal, as it reduces abstraction and increases coupling to a particular implementation. To address this, I attempted to define common option-setting functions. I considered two approaches: 1. Providing a universal function such as `setOption(TransactionOption option, std::string_view value)` that can handle all options. 2. Using separate, specific functions like `enableStaleRead` and `setPriority`. I chose the second approach because it assumes there are not many options to set, and this method is clearer and type-safe compared to passing raw value strings. Thank you for taking the time to review. Feel free to close the MR if it is too small or not helpful. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-26 09:48:23 +00:00

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

deepseek-ai/3FS#297

No description provided.

Rows
Columns