Rework of FDB Wipe + Remote FDB Wipe by ChrisspyB · Pull Request #184 · ecmwf/fdb

ChrisspyB · 2025-10-20T13:13:01Z

Description

Complete rework of the wipe visitor mechanism. We now visit the Catalogue in order to produce a WipeState object, which contains URIs to be deleted/marked as safe. The client then forwards this information to the relevant Stores.
This abstraction should allow for mixed Catalogue/Store backends.
This PR also adds the corresponding functionality for Wiping in Remote FDB.

Some caveats:

There are a couple of TODO's in the code. I will continue chipping away at these.
DAOS wipe functionality is broken. This is currently preventing build on the CI. I will fix things so that they build, but further work will be required to reimplement the DAOS wipe.
--unsafe-wipe-all will hit an assert(false) on remote FDB, as I do not want this functionality supported without better testing / some discussion.
I am currently trying to test the remote functionality.

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 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-184

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

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

Set default for WipeElementType type_ Co-authored-by: Metin Cakircali <11517091+mcakircali@users.noreply.github.com>

Feature/fdb 614 daos wipe refactor

FDB-508 Fix merge of daos wipe changes

…roots

This is of particular relevance when an FDB has been reindexed, as this means there are essentially two catalogues for one store. Extended reindex test to also wipe the FDB.

…few enhancements in wipe tests.

…-changes

Also run clang format

…r-specified

Tests have had to be further updated to account for the bug reported in FDB-633 being fixed, as this has increased the number of index files written when subtocs are used. format

simondsmart

Super happy with this. Only very minor comments, and happy for it to be merged without my re-review after this.

Can you make sure that we include the diagrams that you drew of the overall wipe process/structure, and reference these in an obvious comment at the wipe entry point. There is a lot of implicit overall flow/state knowledge which is needed to work on this code, and we want to give any future developers a heads up...

simondsmart · 2026-02-13T11:19:38Z

src/fdb5/database/WipeCoordinator.cc

+                                                                 [](const auto& pair) { return !pair.second.empty(); });
+
+    if (doit && unclean && !unsafeWipeAll) {
+        eckit::Log::warning() << "Unclean FDB database has the following unknown URIs:" << std::endl;


If we are going to error out here, throwing an exception, then this should probably go to Log::error()

simondsmart · 2026-02-13T11:25:24Z

src/fdb5/api/FDB.cc

+        CatalogueWipeState catalogueWipeState;
+        while (it.next(catalogueWipeState)) {
+
+            auto elements = coordinator.wipe(catalogueWipeState, doit, unsafeWipeAll);


The elements here are constructed inside the coordinator. Consider whether we should just pass the queue to coordinator, and have it push directly? (This may be a bad idea, but it looks to me like the output elements are all constructed after the action has taken place so it should be functionally equivalent?)

I think this is a sensible suggestion

simondsmart · 2026-02-13T11:26:01Z

src/fdb5/api/local/WipeVisitor.h

-#include "eckit/filesystem/PathName.h"

+template <>
+struct std::hash<eckit::URI> {


This probably belongs in eckit in URI.h

It's definitely a bit random to see it in WipeVisitor.h... I wonder if it is still used.

simondsmart · 2026-02-13T11:26:39Z

src/fdb5/api/local/WipeVisitor.h

-#include "eckit/filesystem/PathName.h"

+template <>
+struct std::hash<eckit::URI> {


Note that specialising std::hash (or anything else) is the one thing that should be done inside namespace std!!!

simondsmart · 2026-02-13T11:27:56Z

src/fdb5/api/local/WipeVisitor.h

-#include "eckit/filesystem/PathName.h"

+template <>
+struct std::hash<eckit::URI> {


This should almost certainly be in eckit, in URI.h

Note that specialising a template in namespace std is the one thing that you should be doing inside namesapace std.

simondsmart · 2026-02-13T11:34:12Z

src/fdb5/api/local/WipeVisitor.cc

+        catalogueWipeState_.catalogue(catalogue.config());
+
+        // Build the initial control state (is there really not a function for this?)
+        ControlIdentifiers id;


It doesn't look like it. But do have a look at the constructor ControlElement::ControlElement(const Catalogue&) - which should probably be abstracted to somewhere else.

simondsmart · 2026-02-13T11:35:50Z

src/fdb5/api/local/WipeVisitor.cc

+            catalogueWipeState_.excludeData(dataURI);
+        }
+    }
+    return true;


This returns true, but I don't see us having (or needed) calls to visitDatum anywhere?

simondsmart · 2026-02-13T11:46:05Z

src/fdb5/toc/TocStore.cc

+bool TocStore::doWipeUnknowns(const std::set<eckit::URI>& unknownURIs) const {
+    for (const auto& uri : unknownURIs) {
+        if (uri.path().exists()) {
+            remove(uri, eckit::Log::info(), eckit::Log::info(), true);


Note that we have "using namespace eckit" at the top of this file. We can tidy quite a lot of eckit:: in this file.

simondsmart · 2026-02-13T11:48:20Z

src/fdb5/toc/TocCommon.cc


+//----------------------------------------------------------------------------------------------------------------------
+
+StdDir::StdDir(const eckit::PathName& p) : path_(p), d_(opendir(p.localPath())) {


This is copied from elsewhere, no? Is it not possible to put this somewhere sane for common use?

IIRC FDB's StdDir is confusingly functionally distinct from eckit's StdDir. But I should check.

Adds [CATALOGUE] and [STORE] to the revelevant server logs to allow for better filtering when debugging. Also enable FDB_DEBUG for the server tests, as the serverside logs are otherwise quite sparse and not useful for debugging.

And minor change to remote_api test logging and modify it such that client sleeps while the server flushes the consolidated index.

danovaro and others added 30 commits January 10, 2025 10:46

wip

26679af

wip

6a3e405

Merge branch 'develop' into remoteWipeIterator

91f076d

Merge branch 'develop' into remoteWipeIterator

4f48991

wip

e1db49e

Merge branch 'develop' into remoteWipeIterator

d792902

Merge branch 'develop' into remoteWipeIterator

0a7ac2f

Merge branch 'develop' into remoteWipeIterator

b18250f

wip

a31be04

Merge branch 'develop' into remoteWipeIterator

0ec4821

Merge branch 'develop' into remoteWipeIterator

2fca1ef

wip

32579fb

wip

494321d

Merge branch 'develop' into remoteWipeIterator

f90f3e0

FDB-508 remote wipe

a00d02b

FDB-508 cleanup

846f3ed

FDB-508 committed missing test

def603e

FDB-508 cleanup

6788d08

FDB-508 wipe: added checks for unexpected files

49cb9bf

FDB-508 wipe: added checks for unexpected files

8cfe905

disabled DAOS tests

b3023d0

format

8103bcc

missing include

6b1bcea

version

e89e31c

FDB-508 fix wipe (map of stores & data files)

7e92f92

version

f737567

Merge branch 'develop' into remoteWipeIterator

0438e94

Update src/fdb5/api/helpers/WipeIterator.h

76aff85

Set default for WipeElementType type_ Co-authored-by: Metin Cakircali <11517091+mcakircali@users.noreply.github.com>

Merge branch 'develop' into remoteWipeIterator

8ac4d7d

wip

6aa48d3

Merge pull request #214 from ecmwf/feature/FDB-614-daos-wipe-refactor

b6ceb1e

Feature/fdb 614 daos wipe refactor

ChrisspyB force-pushed the wipe-changes branch from 4b98995 to a4e6af0 Compare January 26, 2026 12:53

FDB-508 minor refactor of fdb wipe internals

51e1f9d

FDB-508 Fix merge of daos wipe changes

ChrisspyB force-pushed the wipe-changes branch from a4e6af0 to 51e1f9d Compare January 26, 2026 12:59

ChrisspyB and others added 14 commits January 26, 2026 17:47

FDB-508 Modify wipe tool tests to test single and separate cat/store …

cb95171

…roots

FDB-508 do not report missing URIs on remote wipe

ea166e7

Merge branch 'develop' into wipe-changes

61a0211

FDB-508 throw exception if Store receieves invalid URIs

f207c73

FDB-508 Restore catalogue control state after wipe

c043de2

FDB-508 Add asserts and formatting

a1f44e2

FDB-508 Update wipe api test to count wipe elements

92774ed

FDB-508 Test that selectfdb.wipe fails on matching multiple lanes

a699b2a

FDB-508 Minor improvements

c9f472a

FDB-508 Fix syscall error when wiping stores with missing directories

fe573fa

This is of particular relevance when an FDB has been reindexed, as this means there are essentially two catalogues for one store. Extended reindex test to also wipe the FDB.

FDB-508 Make WipeState moveable

43c3893

Enabling wipe api test with catalogue and store on separate roots. A …

0f7a1c0

…few enhancements in wipe tests.

Merge branch 'wipe-changes' of https://github.com/ecmwf/fdb into wipe…

ce91924

…-changes

FDB-508 Cache ptr to StoreWipeState on StoreHandler

e75feaa

Also run clang format

ChrisspyB marked this pull request as ready for review February 9, 2026 09:34

danovaro and others added 4 commits February 12, 2026 00:33

Merge branch 'develop' into wipe-changes

2fa8cd4

FDB-508 Wipe tool should fail when requests matches no data or is ove…

7a7b481

…r-specified

FDB-508 RemoteStore uriBelongs always returns false

19c555c

FDB-508 Only wipe masked data on wipe all. Update tests accordingly.

4d554a0

Tests have had to be further updated to account for the bug reported in FDB-633 being fixed, as this has increased the number of index files written when subtocs are used. format

simondsmart approved these changes Feb 13, 2026

View reviewed changes

ChrisspyB added 2 commits February 13, 2026 17:36

FDB-508 Resource lock wipe tests

4f74651

FDB-508 Cleaner logging in remote API test.

b42acf0

Adds [CATALOGUE] and [STORE] to the revelevant server logs to allow for better filtering when debugging. Also enable FDB_DEBUG for the server tests, as the serverside logs are otherwise quite sparse and not useful for debugging.

ChrisspyB removed the request for review from Ozaq February 17, 2026 12:39

FDB-508 Fix lifetime issue of FDBBase on wipe.

3560b41

And minor change to remote_api test logging and modify it such that client sleeps while the server flushes the consolidated index.

ChrisspyB force-pushed the wipe-changes branch from 665faee to 3560b41 Compare February 17, 2026 12:44

Merge branch 'develop' into wipe-changes

75a3607


		//----------------------------------------------------------------------------------------------------------------------

		StdDir::StdDir(const eckit::PathName& p) : path_(p), d_(opendir(p.localPath())) {

Conversation

ChrisspyB commented Oct 20, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Declaration

Uh oh!

simondsmart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ChrisspyB commented Oct 20, 2025 •

edited by github-actions bot

Loading