Skip to content

Docs/fdb 503 envvar#229

Merged
danovaro merged 7 commits intodevelopfrom
docs/FDB-503-envvar
Mar 6, 2026
Merged

Docs/fdb 503 envvar#229
danovaro merged 7 commits intodevelopfrom
docs/FDB-503-envvar

Conversation

@ChrisspyB
Copy link
Copy Markdown
Member

@ChrisspyB ChrisspyB commented Feb 19, 2026

Description

We do the following:

  • Add support for FDB_CONFIG and FDB_CONFIG_FILE where we previously only supported FDB5_CONFIG and FDB5_CONFIG_FILE. The variable without the "5" takes precendence.
  • Removed the server config variables FDB_IS_CAT and FDB_IS_STORE.
  • Added documentation for FDB_CONFIG and FDB_CONFIG_FILE. This should probably also link to a more general documentation page about FDB configuration, if such a thing exists.

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

🌈🌦️📖🚧 Documentation Z3FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/z3fdb/pull-requests/PR-229

🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-229

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 19, 2026

Codecov Report

❌ Patch coverage is 77.58621% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.34%. Comparing base (aa3e626) to head (b82185d).

Files with missing lines Patch % Lines
src/fdb5/remote/FdbServer.cc 18.75% 13 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #229      +/-   ##
===========================================
+ Coverage    71.18%   71.34%   +0.15%     
===========================================
  Files          376      376              
  Lines        23709    23757      +48     
  Branches      2476     2479       +3     
===========================================
+ Hits         16877    16949      +72     
+ Misses        6832     6808      -24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ChrisspyB ChrisspyB mentioned this pull request Feb 19, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates FDB configuration handling and documentation to support the newer FDB_CONFIG / FDB_CONFIG_FILE environment variables (with precedence over the legacy FDB5_* names), and removes (intends to remove) server handler-selection envvars.

Changes:

  • Add FDB_CONFIG and FDB_CONFIG_FILE support (with fallback to FDB5_CONFIG / FDB5_CONFIG_FILE).
  • Update server handler selection to rely on type in config (and add an error on unexpected types).
  • Add new documentation page for FDB configuration-related environment variables and link it from the docs index.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
src/fdb5/remote/FdbServer.cc Adjusts handler selection logic for threaded server mode and adds error handling.
src/fdb5/config/Config.cc Adds new envvar names and precedence for config string / config file selection.
docs/fdb/index.rst Adds the new environment variables page to the FDB docs toctree.
docs/fdb/content/environment-variables.rst Documents FDB_CONFIG and FDB_CONFIG_FILE.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docs/fdb/content/environment-variables.rst Outdated
Comment thread docs/fdb/content/environment-variables.rst
Comment thread docs/fdb/content/environment-variables.rst Outdated
Comment thread src/fdb5/remote/FdbServer.cc
Comment thread src/fdb5/remote/FdbServer.cc Outdated
Comment thread src/fdb5/remote/FdbServer.cc
Comment thread src/fdb5/config/Config.cc
@ChrisspyB
Copy link
Copy Markdown
Member Author

@copilot open a new pull request to apply changes based on the comments in this thread

Copy link
Copy Markdown

Copilot AI commented Feb 20, 2026

@ChrisspyB I've opened a new pull request, #231, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Copy Markdown
Member

@Ozaq Ozaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two additional remarks:

  1. Please also update tests/fdb/api/test_config.cc to test precedence.

  2. PLease reword the commit message, this is more than doc change. This is a(good!) behaviour change but this should be apparent from the commit message that this is not just doc

Comment thread src/fdb5/remote/FdbServer.cc
Comment thread src/fdb5/remote/FdbServer.cc
Comment thread src/fdb5/config/Config.cc
Comment thread docs/fdb/content/environment-variables.rst Outdated
Comment thread docs/fdb/content/environment-variables.rst
@ChrisspyB
Copy link
Copy Markdown
Member Author

ChrisspyB commented Feb 20, 2026

2. PLease reword the commit message, this is more than doc change. This is a(good!) behaviour change but this should be apparent from the commit message that this is not just doc

I separated the doc and non-doc commits!

But I will squash all the commits at the end.

@danovaro danovaro merged commit b52fa0d into develop Mar 6, 2026
156 of 161 checks passed
@danovaro danovaro deleted the docs/FDB-503-envvar branch March 6, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants