remoteFDB - negotiate common set of client & server features - #340
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #340 +/- ##
===========================================
+ Coverage 77.52% 77.58% +0.05%
===========================================
Files 411 411
Lines 27612 27669 +57
Branches 2776 2769 -7
===========================================
+ Hits 21407 21467 +60
+ Misses 6205 6202 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds client/server feature negotiation to remoteFDB handshaking, initially covering wipe availability.
Changes:
- Serializes and intersects feature masks during startup.
- Rejects unsupported client requests before transmission.
- Adjusts remote regression-test ports and startup delays.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/fdb5/remote/Messages.h |
Declares feature-mask helpers and changes header version. |
src/fdb5/remote/Messages.cc |
Implements message-mask conversion and formatting. |
src/fdb5/remote/RemoteConfiguration.h |
Exposes negotiated features and logging support. |
src/fdb5/remote/RemoteConfiguration.cc |
Builds, serializes, and intersects feature masks. |
src/fdb5/remote/client/ClientConnection.h |
Stores negotiated features. |
src/fdb5/remote/client/ClientConnection.cc |
Parses negotiation results and guards requests. |
tests/regressions/FDB-595/FDB-595.sh.in |
Adds a server startup delay. |
tests/regressions/FDB-610/FDB-610.sh.in |
Reduces the startup delay. |
tests/regressions/FDB-610/fdb-remote.yaml |
Updates the catalogue port. |
tests/regressions/FDB-610/fdb_config_catalogue.yaml |
Updates catalogue and store endpoints. |
tests/regressions/FDB-610/fdb_config_store.yaml |
Updates the store port. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| static bool wipeEnabled = eckit::Resource<bool>("fdbWipeEnabled;$FDB_WIPE_ENABLED", true); | ||
| if (wipeEnabled) { | ||
| enabledFeatures_ |= toMask(Message::Wipe); |
| } | ||
|
|
||
| LOG_DEBUG_LIB(LibFdb5) << "Protocol negotiation - NumberOfConnections " << ncSelected << std::endl; | ||
| agreedConf.enabledFeatures_ = clientConf.enabledFeatures_ & serverConf.enabledFeatures_; |
There was a problem hiding this comment.
agreed. pr is missing test that verifies wipe is rejected
023725d to
b42eb17
Compare
mcakircali
left a comment
mcakircali
left a comment
There was a problem hiding this comment.
it would be sensible to add "wipe is rejected" test case
also, you changed server ports of some tests, but seems not relevant to changes in this PR, and those changes were already done in other PR. re-doing work and will cause merge conflict later. I suggest to merge PR 333 to develop and update this PR.
| } | ||
|
|
||
| LOG_DEBUG_LIB(LibFdb5) << "Protocol negotiation - NumberOfConnections " << ncSelected << std::endl; | ||
| agreedConf.enabledFeatures_ = clientConf.enabledFeatures_ & serverConf.enabledFeatures_; |
There was a problem hiding this comment.
agreed. pr is missing test that verifies wipe is rejected
b42eb17 to
4d9ba49
Compare
9a9340c to
e1a2e17
Compare
e1a2e17 to
56ce26d
Compare
Description
extends the remoteFDB protocol handshaking with feature negotiation (currently only addressing wipe support)
Contributor Declaration
By opening this pull request, I affirm the following:
🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/fdb/pull-requests/PR-340