Repository navigation
obj plugin: introduce registry for vendor backends - #1550
Merged
Merged
Conversation
oliviergaraud
requested review from
a team,
brminich,
vvenkates27 and
w1ldptr
as code owners
April 17, 2026 16:48
|
👋 Hi oliviergaraud! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a thread-safe singleton registry for accelerated object-engine implementations; engines self-register via a static registrar. The OBJ backend parses an accelerated Changes
Sequence Diagram(s)sequenceDiagram
participant Backend as Backend
participant Registry as objAccelEngineRegistry
participant Factory as EngineFactory
participant Engine as nixlObjEngineImpl
participant S3 as iS3Client
Backend->>Registry: create(type, init_params[, s3_client, s3_client_crt])
Registry->>Registry: lookupOrThrow(type)
alt factory requires clients
Registry->>Factory: createWithClients(init_params, s3_client, s3_client_crt)
Factory->>Engine: construct(engine with injected S3 clients)
else factory simple
Registry->>Factory: create(init_params)
Factory->>Engine: construct(engine)
end
Engine-->>Backend: return unique_ptr<nixlObjEngineImpl>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/plugins/obj/meson.build (1)
41-74: 🧹 Nitpick | 🔵 Trivial
compile_flagsis applied only in the static build path.Line 65 uses
compile_defs + compile_flagswhile line 74 usescompile_defs + ['-fPIC']and dropscompile_flags. Todaycompile_flagsis empty, so this is inert, but the asymmetry will silently diverge the two build configurations as soon as something is added to it.♻️ Suggested symmetry
- cpp_args: compile_defs + ['-fPIC'], + cpp_args: compile_defs + compile_flags + ['-fPIC'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/obj/meson.build` around lines 41 - 74, The shared-library branch for obj_backend_lib omits compile_flags while the static branch uses compile_defs + compile_flags, which will diverge configurations when compile_flags becomes non-empty; update the shared_library call in the else branch so its cpp_args mirrors the static branch plus position-independent code (e.g., use cpp_args: compile_defs + compile_flags + ['-fPIC'] or prepend compile_flags into the existing list), making sure the symbol names referenced are compile_flags and obj_backend_lib (and the static_plugins conditional) so both static and shared builds receive the same compile_flags.src/plugins/obj/obj_backend.cpp (1)
42-59:⚠️ Potential issue | 🟠 MajorUnguarded registry exceptions will crash the OBJ plugin constructor — wrap in try/catch or return error status.
objAccelEngineRegistry::create(...)throwsstd::runtime_errorwhen the requested type isn't registered (seesrc/plugins/obj/obj_engine_registry.cpplines 26–54). These calls increateObjEngineImpl(lines 43–44 and 56–59) are unguarded; sincecreateObjEngineImplruns fromnixlObjEngine's member initializer (lines 68–69 and 74–75), exceptions escape the constructor. The plugin framework'screateEngineatnixl_agent.cpp:343does not catch them, so a user passing a misspelled or uncompiled accelerated engine type will cause an uncaughtstd::runtime_errorto propagate up the stack.This is a breaking change from the previous
#ifdefflow (which fell back silently). Compare tosrc/plugins/gpunetio/gpunetio_plugin.cpp:25–28, which wraps its constructor in try/catch. Either:
- Catch exceptions here and return
nullptr(letting the caller surfaceNIXL_ERR_BACKEND), or- Confirm the agent/application framework always wraps
createBackendin try/catch (as seen intest/unit/plugins/gusli/nixl_gusli_test.cpp:429–434).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/obj/obj_backend.cpp` around lines 42 - 59, The calls to objAccelEngineRegistry::instance().create(...) inside createObjEngineImpl (guarded by isAcceleratedRequested and using getAccelType) can throw std::runtime_error and are currently unguarded; wrap each registry create call in a try/catch that catches std::exception (or std::runtime_error) and on error log the exception and return nullptr (or otherwise return an error sentinel) so exceptions do not escape the nixlObjEngine constructor; ensure both overloads of createObjEngineImpl handle the exception the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/obj/obj_backend.cpp`:
- Around line 29-38: Change the helper getAccelType to accept a const pointer
since it only reads custom_params: update the function signature to take const
nixl_b_params_t *custom_params and adjust any internal usage to respect
constness (e.g., iterator types or find calls) so the function compiles with
const-qualified arguments; ensure callers of getAccelType still match or are
updated if necessary.
In `@src/plugins/obj/obj_engine_registry.cpp`:
- Around line 17-19: The add() method in objAccelEngineRegistry silently
overwrites existing entries in entries_ causing non-deterministic static-init
collisions; update objAccelEngineRegistry::add to detect duplicates by using
entries_.emplace(type, std::move(entry)) and check the emplace return value, and
if insertion fails either assert or emit a warning/error (e.g., via the
registry's logging mechanism) that the given type was already registered
(include the type name and possibly existing entry info) so duplicate
registrations are detected instead of overwritten.
- Around line 10-54: The file fails the project's clang-format check; reformat
the objAccelEngineRegistry implementations (methods:
objAccelEngineRegistry::instance, add, has, and both create overloads) using the
project's configured clang-format-19 and commit the updated formatting so the
signatures, braces, indentation and line breaks match the project's style; run
clang-format-19 on this file (or the repo) and replace the current contents with
the formatted output before pushing.
- Around line 26-54: The two objAccelEngineRegistry::create overloads duplicate
the same entries_ lookup and error handling; add a private helper method (e.g.,
objAccelEngineRegistry::lookupOrThrow(const std::string& type) const) that finds
the entry in entries_ and throws the existing "not compiled"/runtime_error
messages when missing or when type is empty, then change both create(const
std::string&, const nixlBackendInitParams*) and create(const std::string&, const
nixlBackendInitParams*, std::shared_ptr<iS3Client>, std::shared_ptr<iS3Client>)
to call lookupOrThrow(type) and then invoke entry.create(...) or
entry.createWithClients(...), respectively, to eliminate the duplicated logic
while keeping the same behavior and error texts.
In `@src/plugins/obj/obj_engine_registry.h`:
- Around line 18-57: The header fails clang-format checks; reformat this file
using clang-format-19 to match project style so the typedefs and structs
(obj_accel_create_fn_t, obj_accel_create_with_clients_fn_t,
objAccelEngineEntry), the class objAccelEngineRegistry (instance(), add(),
has(), create() overloads, entries_), and the objAccelEngineRegistrar
constructor are properly indented/wrapped; run clang-format-19 on the file and
commit the resulting changes.
- Around line 31-50: The registry is a mutable global singleton
(objAccelEngineRegistry) and currently accesses entries_ without
synchronization; either document the "register during static init, read at
runtime" contract or make the class thread-safe—preferably add a std::mutex
member (e.g., mutex_) and take a std::lock_guard<std::mutex> in add(), has(),
and both create() overloads to protect accesses to entries_; update the header
to declare the mutex_ and update the implementation of add/has/create to lock it
around reads/writes to entries_ (or alternatively add a brief comment on the
static-init-only requirement and an assert/seal to enforce it).
- Around line 6-7: The header guard macro in obj_engine_registry.h uses
OBJ_ENGINE_REGISTRY_H which doesn't follow the repo convention; change both
occurrences of the guard (the `#ifndef/`#define and the corresponding `#endif`
comment if present) to the repo-standard macro
NIXL_SRC_PLUGINS_OBJ_OBJ_ENGINE_REGISTRY_H so the guard is upper-snake-case
derived from the full path; update occurrences referencing OBJ_ENGINE_REGISTRY_H
in this file (and matching `#endif`) to the new NIXL_... name to ensure uniqueness
and style compliance.
In `@src/plugins/obj/s3_accel/dell/engine_impl.cpp`:
- Around line 18-28: The registrar declaration reg_dell using
objAccelEngineRegistrar and the lambda factories for S3DellObsObjEngineImpl is
misformatted and placed outside the file-local anonymous namespace; run
clang-format-19 to fix formatting and move the static objAccelEngineRegistrar
reg_dell(...) declaration into the existing anonymous namespace (where the other
TU-local helpers live) so the symbol is file-local and grouped with
S3DellObsObjEngineImpl helpers.
In `@src/plugins/obj/s3_accel/engine_impl.cpp`:
- Around line 10-19: Run clang-format-19 on this file to fix the style diffs,
and add a short comment explaining the empty-string registration key used when
constructing reg_s3_accel (e.g., that "" matches the default returned by
getAccelType() when no "type" is set in custom params). Locate the static
registrar symbol reg_s3_accel and the constructor usages of S3AccelObjEngineImpl
in this block and insert the explanatory comment immediately above the registrar
declaration; then reformat the file with clang-format-19.
In `@test/gtest/unit/obj/mock_client_registry.h`:
- Around line 19-36: The header formatting for class mockClientRegistry (methods
instance, add, create) does not match project style; run clang-format-19 on
test/gtest/unit/obj/mock_client_registry.h to reformat the class and its methods
(instance(), add(), create()), then stage the updated file so CI passes the
Clang Format Check.
- Around line 6-7: The header guard macro
TEST_GTEST_UNIT_OBJ_MOCK_CLIENT_REGISTRY_H lacks the required NIXL_ prefix;
update the guard to NIXL_TEST_GTEST_UNIT_OBJ_MOCK_CLIENT_REGISTRY_H in both the
`#ifndef/`#define and corresponding `#endif` comment, ensuring the macro follows the
project's upper-snake-case rule derived from the header's repo-relative path and
that all occurrences (including any include guards or comments referencing it)
are updated to match.
In `@test/gtest/unit/obj/mock_dell_s3_client.cpp`:
- Around line 57-59: The file fails clang-format CI around the static registrar
definition; run clang-format-19 on test/gtest/unit/obj/mock_dell_s3_client.cpp
to fix spacing/linebreaks for the static variable reg_dell and its initializer
(symbols: reg_dell, mockClientRegistrar, mockDellS3Client). Reformat the file
with clang-format-19 and commit the resulting changes so the registrar
definition matches the project's style.
In `@test/gtest/unit/obj/mock_s3_client.h`:
- Around line 29-34: The ctor mockS3Client and setExecutor currently do a
std::dynamic_pointer_cast to asioThreadPoolExecutor and silently store nullptr
in executor_, which later causes execAsync() to crash; change both places (the
mockS3Client constructor and setExecutor) to check the result of the
dynamic_pointer_cast and fail fast if it returns nullptr (e.g., use an assertion
or throw a clear runtime_error) so tests immediately surface passing a
non-asioThreadPoolExecutor rather than crashing later in execAsync().
- Around line 78-85: The execAsync() implementation dereferences executor_
without checking for null; update execAsync() (which iterates pendingCallbacks_
and calls executor_->Submit(...) and executor_->waitUntilIdle()) to guard
against a null executor_ (set via setExecutor()); either throw a clear
std::runtime_error or assert with a descriptive message when executor_ is null,
or skip submitting callbacks and only clear pendingCallbacks_ and return when
executor_ is null—choose one of these behaviors and apply it consistently so
execAsync() no longer segfaults when the parameterless mockS3Client constructor
is used and setExecutor() was never called.
In `@test/gtest/unit/obj/obj.cpp`:
- Around line 65-70: The test silently falls back to base mock when
mockClientRegistry::instance().create(type) returns nullptr for a non-empty
customParams_["type"], masking missing registrar/link issues; update the fixture
to check the local variable type and, if type is non-empty and
mockClientRegistry::instance().create(type) returns nullptr, fail the test with
a clear assertion/error (mentioning the missing registered mock for that type)
instead of silently using mockS3Client_, otherwise allow the existing fallback
when type is empty; modify the code around the create(type) call and the
mockS3Client_ assignment and ensure objEngine_ construction remains unchanged.
---
Outside diff comments:
In `@src/plugins/obj/meson.build`:
- Around line 41-74: The shared-library branch for obj_backend_lib omits
compile_flags while the static branch uses compile_defs + compile_flags, which
will diverge configurations when compile_flags becomes non-empty; update the
shared_library call in the else branch so its cpp_args mirrors the static branch
plus position-independent code (e.g., use cpp_args: compile_defs + compile_flags
+ ['-fPIC'] or prepend compile_flags into the existing list), making sure the
symbol names referenced are compile_flags and obj_backend_lib (and the
static_plugins conditional) so both static and shared builds receive the same
compile_flags.
In `@src/plugins/obj/obj_backend.cpp`:
- Around line 42-59: The calls to objAccelEngineRegistry::instance().create(...)
inside createObjEngineImpl (guarded by isAcceleratedRequested and using
getAccelType) can throw std::runtime_error and are currently unguarded; wrap
each registry create call in a try/catch that catches std::exception (or
std::runtime_error) and on error log the exception and return nullptr (or
otherwise return an error sentinel) so exceptions do not escape the
nixlObjEngine constructor; ensure both overloads of createObjEngineImpl handle
the exception the same way.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43c5ad3e-6753-427a-988a-741c804a1d1e
📒 Files selected for processing (13)
src/plugins/obj/README.mdsrc/plugins/obj/meson.buildsrc/plugins/obj/obj_backend.cppsrc/plugins/obj/obj_engine_registry.cppsrc/plugins/obj/obj_engine_registry.hsrc/plugins/obj/s3_accel/dell/engine_impl.cppsrc/plugins/obj/s3_accel/engine_impl.cppsrc/utils/object/engine_utils.htest/gtest/unit/obj/meson.buildtest/gtest/unit/obj/mock_client_registry.htest/gtest/unit/obj/mock_dell_s3_client.cpptest/gtest/unit/obj/mock_s3_client.htest/gtest/unit/obj/obj.cpp
💤 Files with no reviewable changes (1)
- src/utils/object/engine_utils.h
oliviergaraud
force-pushed
the
plugin_obj_registry
branch
from
April 17, 2026 17:31
46d6e19 to
b95cef2
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/obj/README.md (1)
358-358:⚠️ Potential issue | 🟡 MinorExample vendor headers use
#pragma once, contradicting project style.The sample
client.h(line 358) andengine_impl.h(line 387) use#pragma once, but NIXL C++ style mandates traditional#ifndef/#defineheader guards. Since this README is the onboarding reference new vendor authors will copy verbatim, these snippets will seed non-compliant code.As per coding guidelines: "use traditional
#ifndef/#define header guards (no#pragmaonce) with an upper-snake-case name derived from the file path".📝 Proposed fix
**client.h** ```cpp -#pragma once +#ifndef NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_CLIENT_H +#define NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_CLIENT_H `#include` "s3_accel/client.h" … +#endif // NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_CLIENT_Hengine_impl.h
-#pragma once +#ifndef NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_ENGINE_IMPL_H +#define NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_ENGINE_IMPL_H `#include` "s3_accel/engine_impl.h" … +#endif // NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_ENGINE_IMPL_H</details> Also applies to: 387-387 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@src/plugins/obj/README.mdat line 358, The README examples use "#pragma
once" which violates NIXL C++ style; replace the pragma in the vendor sample
headers (client.h and engine_impl.h) with traditional include guards: add an
#ifndef/#define pair at the top and a matching#endifat the bottom using an
upper-snake-case name derived from the file path (use
NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_CLIENT_H for client.h and
NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_ENGINE_IMPL_H for engine_impl.h), and keep
all existing includes and content between the guard macros.</details> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@src/plugins/obj/obj_backend.cpp`: - Around line 43-51: Extract the duplicated try/catch/log/rethrow into a small helper function (e.g., createAccelEngine) and use it from both createObjEngineImpl overloads: have createAccelEngine accept the accel type and init_params (or just init_params and call getAccelType inside) and perform objAccelEngineRegistry::instance().create(... ) inside a single try block, catch const std::exception& e, call NIXL_ERROR with the error message, then rethrow; replace the two duplicated try/catch blocks in createObjEngineImpl with a single call to createAccelEngine to centralize logging and error handling. In `@src/plugins/obj/obj_engine_registry.cpp`: - Line 19: The local std::lock_guard usages should be declared const for const-correctness: replace occurrences of "std::lock_guard<std::mutex> lock(mutex_);" with "const std::lock_guard<std::mutex> lock(mutex_);" (apply the same change to the other two lock declarations in the file) so all local lock_guard locals (e.g., the variables named lock that protect mutex_) are const-qualified. - Around line 33-44: lookupOrThrow currently returns a const reference to an element from the unordered_map entries_ after releasing mutex_, which is unsafe because concurrent add() can rehash and invalidate the reference; modify objAccelEngineRegistry::lookupOrThrow to return objAccelEngineEntry by value instead (copy the found entry while holding mutex_ and return the copy) or, if you prefer keeping references, perform the factory invocation (e.g., call .create()/.createWithClients()) while still holding mutex_ to avoid returning an invalid reference; update the function signature and implementation in objAccelEngineRegistry accordingly. In `@src/plugins/obj/obj_engine_registry.h`: - Around line 19-24: The two type aliases obj_accel_create_fn_t and obj_accel_create_with_clients_fn_t violate the project's lower camelCase type naming; rename them to objAccelCreateFn and objAccelCreateWithClientsFn respectively, preserving their signatures (returning std::unique_ptr<nixlObjEngineImpl> and taking const nixlBackendInitParams* and optionally std::shared_ptr<iS3Client> twice); update all local references (including in objAccelEngineEntry and objAccelEngineRegistry) and any forward declarations or headers that use these aliases so builds and signatures remain consistent. In `@src/plugins/obj/README.md`: - Line 346: The README line advising to guard cuobjclient-specific code should use the idiomatic preprocessor forms; update the example `#if defined HAVE_CUOBJ_CLIENT` to either `#ifdef HAVE_CUOBJ_CLIENT` for the simple case or `#if defined(HAVE_CUOBJ_CLIENT)` when combined with other conditions so vendor authors follow the canonical style when guarding code with HAVE_CUOBJ_CLIENT. - Around line 456-468: The README example shows registrar lambdas using the fixed factory signature (params, s3, s3_crt) via objAccelEngineRegistrar, but step 2 and the VendorObjEngineImpl constructors are inconsistent (the documented second ctor takes (const nixlBackendInitParams*, std::shared_ptr<iS3Client>) while the registrar factory supplies s3_crt); update the docs to explicitly state that accel engines and VendorObjEngineImpl typically ignore the CRT client (iS3Client s3_crt), clarify that the registrar's createWithClients factory must accept (p, s3, s3_crt) even if s3_crt is unused, and either align the example constructor signature with the real VendorObjEngineImpl constructors or add a note explaining why the ctor drops s3_crt so copy-pasters know to accept but not use s3_crt. In `@src/plugins/obj/s3_accel/engine_impl.cpp`: - Around line 12-18: The registrar reg_s3_accel (which constructs S3AccelObjEngineImpl using nixlBackendInitParams and iS3Client) is currently declared as a file-scope static; move its declaration into an anonymous namespace to match the sibling file and project style: remove the file-scope static placement and wrap the reg_s3_accel definition inside namespace { ... } so it remains internal linkage but follows the anonymous-namespace convention. In `@test/gtest/unit/obj/mock_dell_s3_client.cpp`: - Around line 20-32: The parameters key, data_ptr, data_len, and offset in putObjectRdmaAsync (and key in getObjectRdmaAsync) are unused; mark them as [[maybe_unused]] (or remove their names) to match the constructor-style hygiene in mock_s3_client.h. Update the function signatures for putObjectRdmaAsync and getObjectRdmaAsync to annotate the unused parameters with [[maybe_unused]] (or drop the identifier) so the compiler no longer warns about unused parameters while preserving the existing behavior and callback logic. - Around line 10-59: Move the mockDellS3Client class and the reg_dell registrar into a TU-local anonymous namespace instead of relying on static linkage; locate the declarations of mockDellS3Client and the static mockClientRegistrar reg_dell and wrap them in `namespace { ... }` so both the class and the registrar become internal to the translation unit, mirroring the pattern used by s3_accel/dell/engine_impl.cpp. In `@test/gtest/unit/obj/mock_s3_client.h`: - Around line 53-59: The parameter list for putObjectAsync (and any similar mocks like getObjectAsync) declares key, data_ptr, data_len, offset but never uses them, which can trigger -Wunused-parameter; update the function signatures for putObjectAsync (and getObjectAsync if present) to mark unused parameters with [[maybe_unused]] (e.g., [[maybe_unused]] std::string_view key, [[maybe_unused]] uintptr_t data_ptr, etc.) so the compiler won’t warn, leaving the existing pendingCallbacks_ lambda and behavior unchanged. - Around line 6-7: The header guard macro in mock_s3_client.h uses TEST_GTEST_UNIT_OBJ_MOCK_S3_CLIENT_H which doesn't follow the repo convention; update both occurrences of the guard (the `#ifndef` and the corresponding `#define` for mock_s3_client.h) to the canonical form NIXL_TEST_GTEST_UNIT_OBJ_MOCK_S3_CLIENT_H and ensure the trailing `#endif` (in the same file) optionally comments the macro name for clarity; locate the macro by the existing symbol TEST_GTEST_UNIT_OBJ_MOCK_S3_CLIENT_H and replace it with NIXL_TEST_GTEST_UNIT_OBJ_MOCK_S3_CLIENT_H. --- Outside diff comments: In `@src/plugins/obj/README.md`: - Line 358: The README examples use "#pragma once" which violates NIXL C++ style; replace the pragma in the vendor sample headers (client.h and engine_impl.h) with traditional include guards: add an `#ifndef/`#define pair at the top and a matching `#endif` at the bottom using an upper-snake-case name derived from the file path (use NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_CLIENT_H for client.h and NIXL_PLUGINS_OBJ_S3_ACCEL_VENDOR_NAME_ENGINE_IMPL_H for engine_impl.h), and keep all existing includes and content between the guard macros.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID:
705ef14e-f80e-48fb-9444-65b44e2072c7📒 Files selected for processing (13)
src/plugins/obj/README.mdsrc/plugins/obj/meson.buildsrc/plugins/obj/obj_backend.cppsrc/plugins/obj/obj_engine_registry.cppsrc/plugins/obj/obj_engine_registry.hsrc/plugins/obj/s3_accel/dell/engine_impl.cppsrc/plugins/obj/s3_accel/engine_impl.cppsrc/utils/object/engine_utils.htest/gtest/unit/obj/meson.buildtest/gtest/unit/obj/mock_client_registry.htest/gtest/unit/obj/mock_dell_s3_client.cpptest/gtest/unit/obj/mock_s3_client.htest/gtest/unit/obj/obj.cpp💤 Files with no reviewable changes (1)
- src/utils/object/engine_utils.h
oliviergaraud
force-pushed
the
plugin_obj_registry
branch
from
April 17, 2026 18:10
b95cef2 to
3219007
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
oliviergaraud
force-pushed
the
plugin_obj_registry
branch
from
April 17, 2026 18:31
3219007 to
e76d5d2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
test/gtest/unit/obj/mock_dell_s3_client.cpp (2)
12-57: 🛠️ Refactor suggestion | 🟠 MajorMove TU-local
mockDellS3Clientandreg_dellinto an anonymous namespace.Both symbols are strictly TU-local test helpers. The production-side registrar in
src/plugins/obj/s3_accel/dell/engine_impl.cppalready uses an anonymous namespace; mirror that here instead of relying onstaticlinkage onreg_delland giving the class external linkage.♻️ Proposed change
namespace gtest::obj { +namespace { class mockDellS3Client : public mockS3Client, public iDellS3RdmaClient { ... }; -static mockClientRegistrar reg_dell("dell", []() { return std::make_shared<mockDellS3Client>(); }); +mockClientRegistrar reg_dell("dell", []() { return std::make_shared<mockDellS3Client>(); }); +} // namespace } // namespace gtest::obj🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gtest/unit/obj/mock_dell_s3_client.cpp` around lines 12 - 57, The TU-local test helpers mockDellS3Client and reg_dell should be placed in an anonymous namespace to give them internal linkage; wrap the mockDellS3Client class definition and the reg_dell registration inside namespace { ... } and remove reliance on file-scope static for reg_dell (keep the registrar creation but move it inside the anonymous namespace) so both symbols become TU-local just like the production registrar in engine_impl.cpp.
20-40: 🧹 Nitpick | 🔵 TrivialMark unused parameters
[[maybe_unused]].
keyin both overrides, anddata_ptr/data_len/offsetinputObjectRdmaAsync, are unused. Annotate them for consistency with thenixl_b_params_t *custom_paramspattern used at line 16 and to keep-Wunused-parameter-clean builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gtest/unit/obj/mock_dell_s3_client.cpp` around lines 20 - 40, The overrides putObjectRdmaAsync and getObjectRdmaAsync declare parameters that are not used; mark the unused parameters with the [[maybe_unused]] attribute (e.g., std::string_view key, uintptr_t data_ptr, size_t data_len, size_t offset, std::string_view rdma_desc as appropriate) to suppress -Wunused-parameter warnings and match the existing pattern used for nixl_b_params_t *custom_params; update the parameter declarations in the putObjectRdmaAsync and getObjectRdmaAsync method signatures accordingly while leaving callback usage unchanged.src/plugins/obj/obj_engine_registry.h (1)
19-24: 🧹 Nitpick | 🔵 TrivialType aliases still use snake_case +
_tsuffix.
obj_accel_create_fn_tandobj_accel_create_with_clients_fn_tremain inconsistent with surroundingobjAccelEngineEntry/objAccelEngineRegistry/objAccelEngineRegistrarcamelCase naming. ConsiderobjAccelCreateFnandobjAccelCreateWithClientsFn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/obj/obj_engine_registry.h` around lines 19 - 24, Rename the snake_case `_t` typedefs to camelCase to match surrounding symbols: change obj_accel_create_fn_t to objAccelCreateFn and obj_accel_create_with_clients_fn_t to objAccelCreateWithClientsFn; update the type definitions in obj_engine_registry.h (the std::function signatures returning std::unique_ptr<nixlObjEngineImpl>) and then update every usage/forward declaration/reference (e.g., in objAccelEngineEntry, objAccelEngineRegistry, objAccelEngineRegistrar and any factory/registration code) to use the new names so identifiers remain consistent across the codebase.test/gtest/unit/obj/mock_s3_client.h (1)
52-66: 🧹 Nitpick | 🔵 TrivialMark unused parameters in
putObjectAsync/getObjectAsyncas[[maybe_unused]].
putObjectAsyncnever useskey,data_ptr,data_len, oroffset;getObjectAsyncnever useskey. Annotate them to match the constructor style on line 30 and avoid-Wunused-parameterregressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gtest/unit/obj/mock_s3_client.h` around lines 52 - 66, Annotate unused function parameters with [[maybe_unused]] to avoid -Wunused-parameter warnings: update the signatures of putObjectAsync (mark key, data_ptr, data_len, offset as [[maybe_unused]]) and getObjectAsync (mark key as [[maybe_unused]]), matching the constructor style used earlier; keep the body logic unchanged (retaining pendingCallbacks_ push in putObjectAsync and existing getObjectAsync implementation). Ensure only the parameter declarations change so callers and overrides remain compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/plugins/obj/obj_engine_registry.h`:
- Around line 19-24: Rename the snake_case `_t` typedefs to camelCase to match
surrounding symbols: change obj_accel_create_fn_t to objAccelCreateFn and
obj_accel_create_with_clients_fn_t to objAccelCreateWithClientsFn; update the
type definitions in obj_engine_registry.h (the std::function signatures
returning std::unique_ptr<nixlObjEngineImpl>) and then update every
usage/forward declaration/reference (e.g., in objAccelEngineEntry,
objAccelEngineRegistry, objAccelEngineRegistrar and any factory/registration
code) to use the new names so identifiers remain consistent across the codebase.
In `@test/gtest/unit/obj/mock_dell_s3_client.cpp`:
- Around line 12-57: The TU-local test helpers mockDellS3Client and reg_dell
should be placed in an anonymous namespace to give them internal linkage; wrap
the mockDellS3Client class definition and the reg_dell registration inside
namespace { ... } and remove reliance on file-scope static for reg_dell (keep
the registrar creation but move it inside the anonymous namespace) so both
symbols become TU-local just like the production registrar in engine_impl.cpp.
- Around line 20-40: The overrides putObjectRdmaAsync and getObjectRdmaAsync
declare parameters that are not used; mark the unused parameters with the
[[maybe_unused]] attribute (e.g., std::string_view key, uintptr_t data_ptr,
size_t data_len, size_t offset, std::string_view rdma_desc as appropriate) to
suppress -Wunused-parameter warnings and match the existing pattern used for
nixl_b_params_t *custom_params; update the parameter declarations in the
putObjectRdmaAsync and getObjectRdmaAsync method signatures accordingly while
leaving callback usage unchanged.
In `@test/gtest/unit/obj/mock_s3_client.h`:
- Around line 52-66: Annotate unused function parameters with [[maybe_unused]]
to avoid -Wunused-parameter warnings: update the signatures of putObjectAsync
(mark key, data_ptr, data_len, offset as [[maybe_unused]]) and getObjectAsync
(mark key as [[maybe_unused]]), matching the constructor style used earlier;
keep the body logic unchanged (retaining pendingCallbacks_ push in
putObjectAsync and existing getObjectAsync implementation). Ensure only the
parameter declarations change so callers and overrides remain compatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fec83b7-b1bb-48b9-82a9-e221459cc4e1
📒 Files selected for processing (13)
src/plugins/obj/README.mdsrc/plugins/obj/meson.buildsrc/plugins/obj/obj_backend.cppsrc/plugins/obj/obj_engine_registry.cppsrc/plugins/obj/obj_engine_registry.hsrc/plugins/obj/s3_accel/dell/engine_impl.cppsrc/plugins/obj/s3_accel/engine_impl.cppsrc/utils/object/engine_utils.htest/gtest/unit/obj/meson.buildtest/gtest/unit/obj/mock_client_registry.htest/gtest/unit/obj/mock_dell_s3_client.cpptest/gtest/unit/obj/mock_s3_client.htest/gtest/unit/obj/obj.cpp
💤 Files with no reviewable changes (1)
- src/utils/object/engine_utils.h
oliviergaraud
force-pushed
the
plugin_obj_registry
branch
from
April 17, 2026 21:42
e76d5d2 to
2eff9b8
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/plugins/obj/obj_engine_registry.h (1)
19-28: 🧹 Nitpick | 🔵 TrivialRename the factory aliases to lowerCamelCase.
obj_accel_create_fn_tandobj_accel_create_with_clients_fn_tstill violate the repo type-naming convention. Prefer names likeobjAccelCreateFnandobjAccelCreateWithClientsFn, then update their uses inobjAccelEngineEntryandobjAccelEngineRegistrar. As per coding guidelines: “Use lowerCamelCase for types/functions and snake_case for variables/locals.”♻️ Proposed rename
-using obj_accel_create_fn_t = +using objAccelCreateFn = std::function<std::unique_ptr<nixlObjEngineImpl>(const nixlBackendInitParams *)>; -using obj_accel_create_with_clients_fn_t = +using objAccelCreateWithClientsFn = std::function<std::unique_ptr<nixlObjEngineImpl>(const nixlBackendInitParams *, std::shared_ptr<iS3Client>, std::shared_ptr<iS3Client>)>; struct objAccelEngineEntry { - obj_accel_create_fn_t create; - obj_accel_create_with_clients_fn_t createWithClients; + objAccelCreateFn create; + objAccelCreateWithClientsFn createWithClients; }; @@ objAccelEngineRegistrar(const std::string &type, - obj_accel_create_fn_t create, - obj_accel_create_with_clients_fn_t create_with_clients) { + objAccelCreateFn create, + objAccelCreateWithClientsFn create_with_clients) {Also applies to: 58-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/obj/obj_engine_registry.h` around lines 19 - 28, The type alias names obj_accel_create_fn_t and obj_accel_create_with_clients_fn_t violate the repo's lowerCamelCase type/function naming rule; rename them to objAccelCreateFn and objAccelCreateWithClientsFn and update all usages (notably in the struct objAccelEngineEntry and the registrar objAccelEngineRegistrar) to use the new identifiers, ensuring function signatures remain unchanged (std::function returning std::unique_ptr<nixlObjEngineImpl> with the same parameters).test/gtest/unit/obj/mock_dell_s3_client.cpp (1)
6-8:⚠️ Potential issue | 🟡 MinorRun clang-format on this file before merging.
The Clang Format Check pipeline still reports
mock_dell_s3_client.cppdiffers fromclang-format-19output. Reformat the file rather than hand-adjusting specific lines. Based on learnings: “When reviewing CI clang-format failures, verify the exact file and line numbers from the CI logs before commenting.”Also applies to: 58-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gtest/unit/obj/mock_dell_s3_client.cpp` around lines 6 - 8, Run the repository's clang-format (the same version used by CI, e.g., clang-format-19) on test/gtest/unit/obj/mock_dell_s3_client.cpp and commit the reformatted file rather than hand-editing lines; ensure the includes shown ("mock_s3_client.h", "mock_client_registry.h", "s3_accel/dell/rdma_interface.h") are formatted according to the project style so the clang-format check in CI passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/plugins/obj/obj_engine_registry.h`:
- Around line 19-28: The type alias names obj_accel_create_fn_t and
obj_accel_create_with_clients_fn_t violate the repo's lowerCamelCase
type/function naming rule; rename them to objAccelCreateFn and
objAccelCreateWithClientsFn and update all usages (notably in the struct
objAccelEngineEntry and the registrar objAccelEngineRegistrar) to use the new
identifiers, ensuring function signatures remain unchanged (std::function
returning std::unique_ptr<nixlObjEngineImpl> with the same parameters).
In `@test/gtest/unit/obj/mock_dell_s3_client.cpp`:
- Around line 6-8: Run the repository's clang-format (the same version used by
CI, e.g., clang-format-19) on test/gtest/unit/obj/mock_dell_s3_client.cpp and
commit the reformatted file rather than hand-editing lines; ensure the includes
shown ("mock_s3_client.h", "mock_client_registry.h",
"s3_accel/dell/rdma_interface.h") are formatted according to the project style
so the clang-format check in CI passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26cc2364-ea27-45fb-a456-49b508cd4fd9
📒 Files selected for processing (13)
src/plugins/obj/README.mdsrc/plugins/obj/meson.buildsrc/plugins/obj/obj_backend.cppsrc/plugins/obj/obj_engine_registry.cppsrc/plugins/obj/obj_engine_registry.hsrc/plugins/obj/s3_accel/dell/engine_impl.cppsrc/plugins/obj/s3_accel/engine_impl.cppsrc/utils/object/engine_utils.htest/gtest/unit/obj/meson.buildtest/gtest/unit/obj/mock_client_registry.htest/gtest/unit/obj/mock_dell_s3_client.cpptest/gtest/unit/obj/mock_s3_client.htest/gtest/unit/obj/obj.cpp
💤 Files with no reviewable changes (1)
- src/utils/object/engine_utils.h
Replace the #ifdef ladder in obj_backend.cpp with a self-registration pattern. Each accelerated engine (S3Accel, Dell) registers itself via a static objAccelEngineRegistrar, and obj_backend.cpp dispatches through the registry. This makes it trivial to add new engines without modifying the factory function. Extract mock S3 client classes into separate headers for reuse, and introduce a mock client registry mirroring the production engine registry pattern. Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
oliviergaraud
force-pushed
the
plugin_obj_registry
branch
from
April 20, 2026 08:26
2eff9b8 to
cfa7505
Compare
ofer
previously approved these changes
Apr 28, 2026
Add braces around single-line if bodies to comply with the project's clang-format configuration. Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
Contributor
|
/ok to test ea867bb |
Contributor
|
@oliviergaraud can you rebase to pick up some CI fixes? |
Contributor
Author
done |
Contributor
|
/ok to test dcd9ea4 |
Contributor
|
/build |
Contributor
|
/build |
2 similar comments
Contributor
|
/build |
Contributor
|
/build |
Contributor
|
@oliviergaraud can you rebase your PRs to latest main? |
Contributor
|
/build |
Contributor
Author
done |
Contributor
|
/ok to test 72343a0 |
ofer
approved these changes
Jun 5, 2026
Contributor
|
/build |
ovidiusm
approved these changes
Jun 5, 2026
This was referenced Oct 6, 2026
This branch was previously deployed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
What?
Implement a self-registration pattern for accelerated OBJ engines.
Replace the
#ifdefladder inobj_backend.cppwith a self-registrationpattern for accelerated OBJ engines (S3Accel, Dell ObjectScale).
Extract mock S3 client classes into reusable headers and mirror the registry pattern on the test side.
Why?
To handle
// TODO: Consider a registration pattern as more vendor engines are added.Adding a new vendor engine required modifying
obj_backend.cpp(two factory overloads,#ifdefguards, includes), which was error-prone and can caused merge conflicts when multiple engines were developed in parallel.With the registry, new engines are self-contained: implement, add a static registrar, update
meson.buildHow?
objAccelEngineRegistrysingleton maps engine type strings to factorylambdas;
objAccelEngineRegistrarRAII helper handles static-init registration from each engine's.cpp.obj_backend.cppdelegates to the registry whenaccelerated=true, removing all vendor-specific code.mockClientRegistrymirrors the pattern for mock S3 client creation.Summary by CodeRabbit
New Features
Refactor
Build
Tests
Documentation