Wipe refactoring + Toc/Remote Wipe #158

danovaro · 2025-08-02T20:49:37Z

Description

The aim is to unify the WipeVisitor and expose some catalogue and store methods responsible for the backend-specific checks and removal actions.

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-158

…k-md and --check-full.

…s enabled.

…trieve.

Set default for WipeElementType type_ Co-authored-by: Metin Cakircali <[email protected]>

…s. Abort list polling if too many retries.

simondsmart

I've added far too many comments to this PR. And I think that it could get lost in the weeds. My overall comments:

The breakdown of Catalogue/store functionality looks good.
Clearly we need to have a mutable Catalogue object through this process. This probably means creating a base EntryVisitor class with a non-const, private catalogue_entry, accessible to two derived classes MutableEntryVisitor and ImmutableEntryVisitor (or similar) which access a protected catalogue() which returns a reference differing in constness.
This wipe runs fully on the catalogue, dispatching to the stores. But this does not work in all cases. Consider a local disk-based store, or OpenFAM store - the remote catalogue has neither access, nor filesystem permissions to make modifications to the store. As such, control here needs to be routed through the client process. This may require the catalogue to sign change requests to the store (or at least, to have a placeholder for doing this now).
I'm not sure that WipeElements is the correct structure to be storing for state to be communicated around this process. That is for communicating back to clients through the API - and it feels a bit forced in places.
I can see why we had a second-level visitor pattern, as there is state which needs to be held. Pushing this all to be persistent state in the Catalogue object is extremely ugly, and makes our thread-nastiness even worse. I think that we need, fundamentally, some type of class-specific context object. Probably a friend object. Which stores that persistent state through the wipe process. Should be easy to extract.

simondsmart · 2025-09-24T15:52:38Z

src/fdb5/api/helpers/ListIterator.h

 #ifndef fdb5_ListIterator_H
 #define fdb5_ListIterator_H

+#include <unordered_map>


Agreed. I'm very surprised to see this being added to this header when the list functionality should be essentially untouched here...

simondsmart · 2025-09-24T15:54:07Z

src/fdb5/api/helpers/WipeIterator.cc

+}
+
+size_t encodeSizeString(const std::string& s) {
+    return (1 + 4 + s.size());


What is the 1 and the 4 for?

simondsmart · 2025-09-24T15:54:28Z

src/fdb5/api/helpers/WipeIterator.cc

+size_t WipeElement::encodeSize() const {
+    size_t aux = 0;
+    for (const auto& l : uris_) {
+        aux += 256;


Surely this should add the length of the URI? Or something dependent on that.

simondsmart · 2025-09-24T15:59:36Z

WipeIterator.cc

+
+void WipeElement::print(std::ostream& out) const {
+
+    LOG_DEBUG_LIB(LibFdb5) << "Wipe(type=" << type_ << ",msg=" << msg_ << ",uris=[";


Oof. This is unpleasant inside ::print(), as this method is typically called in the middle of ostream output. Which results in info/error/debug output becoming interleaved on the terminal. A but unpleasant.

simondsmart · 2025-09-24T16:02:54Z

src/fdb5/api/local/WipeVisitor.cc

+
+        // If the catalogue cannot be wiped, then we don't proceed.
+        // This is a safeguard against wiping a catalogue that is not in a state to be wiped.
+        return false;


The entry visitors can recurse all the way down to elements. If you return false from visitDatabase it says to the visit mechanism not to visit the indexes or the entries explicitly. i.e. stop here.

simondsmart · 2025-09-26T10:35:12Z

src/fdb5/database/Catalogue.h

    void deselectIndex() override { NOTIMP; }

-    std::vector<eckit::PathName> metadataPaths() const override { NOTIMP; }
+    // std::vector<eckit::PathName> metadataPaths() const override { NOTIMP; }


If not needed, delete.

simondsmart · 2025-09-26T10:37:01Z

src/fdb5/daos/DaosWipeVisitor.cc

-                                 const metkit::mars::MarsRequest& request, std::ostream& out, bool doit, bool porcelain,
-                                 bool unsafeWipeAll) :
-    WipeVisitor(request, out, doit, porcelain, unsafeWipeAll), catalogue_(catalogue), store_(store), dbKvName_("") {}
+DaosWipeVisitor::DaosWipeVisitor(const DaosCatalogue& catalogue, const metkit::mars::MarsRequest& request,


This worries me.

Most of the other WipeVisitors have been removed from the code. I think correctly.

But here, we seem to have just commented most of the functionality out. So I suspect that this is just broken.

simondsmart · 2025-09-26T10:37:27Z

src/fdb5/daos/DaosStore.cc

+    return true;
+}
+bool DaosStore::doWipe(const std::vector<eckit::URI>& unknownURIs) const {
+    return true;


doWipe is really a NOP?

simondsmart · 2025-09-26T13:05:02Z

src/fdb5/toc/TocCatalogue.cc

+
+    eckit::URI locationURI{index.location().uri()};
+
+    if (!locationURI.path().dirName().sameAs(basePath())) {


simondsmart · 2025-09-26T15:13:21Z

src/fdb5/api/RemoteFDB.cc

    return forwardApiCall(StatsHelper(), request);
 }

+WipeIterator RemoteFDB::wipe(const FDBToolRequest& request, bool doit, bool porcelain, bool unsafeWipeAll) {


My understanding of this is that it implies that wipe runs fully on the Catalogue (none of the communication is via the client).

What happens in the case that we have a remote Catalogue, but local stores (e.g. stores on Lustre). The Catalogue may not have the right filesystem permissions to wipe data which has been written by the user.

I'm not sure that this design is functionally correct.

…ain.

…rification

…o change

nicolau-manubens and others added 30 commits November 5, 2024 00:31

Implemented two consistency check modes which can enabled with --chec…

0e05001

…k-md and --check-full.

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

FDB-433 fdb-hammer modifications for ITT.

8496aa8

Drafted barrier mechanism for writers.

87d4689

Fixes for fdb-hammer ITT mode.

0047c81

Merge latest develop.

cc37840

Got internode barrier to work plus other fixes.

5bdb2ca

Barrier working at scale.

ec10a2d

Fixes for ITT read mode.

a2e353d

Fixes for ITT mode.

ec1f383

A few improvements.

8c827ad

Randomising data in fdb-hammer writers.

8393aab

Merge branch 'develop' into remoteWipeIterator

b18250f

wip

a31be04

Make MessageArchiver display bytes and time elapsed only if verbose i…

5a031aa

…s enabled.

Merge branch 'develop' into remoteWipeIterator

0ec4821

Modified fdb-hammer to generate 32-bit instead of 64-bit random values.

7f8a896

Merge branch 'develop' into remoteWipeIterator

2fca1ef

fdb-hammer setting random values with codes_set_float_array.

70e4cda

Fixes in fdb-hammer random value generation.

cabb779

Fix in fdb-hammer random data generation.

5b1e5d2

wip

32579fb

wip

494321d

Merge branch 'develop' into remoteWipeIterator

f90f3e0

nicolau-manubens and others added 6 commits September 23, 2025 08:04

fdb-hammer logging message rather than throwing if step window exceeded.

55e8daa

fdb-hammer: Instrument list and bulk data read.

b3055b8

Using FDB::read on previously listed URIs instead of ::list plus ::re…

cc3d948

…trieve.

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

76aff85

Set default for WipeElementType type_ Co-authored-by: Metin Cakircali <[email protected]>

fdb-hammer ITT: add option to read fields from a list of provided URI…

a3d0af5

…s. Abort list polling if too many retries.

fdb-hammer fixes in URI reading.

a8073ef

simondsmart requested changes Sep 26, 2025

View reviewed changes

nicolau-manubens and others added 23 commits September 26, 2025 21:45

fdb-hammer ITT mode fix pre-listing and read bandwidth.

ce0ce86

Merge branch 'release/5.18.0'

9866dfe

Merge branch 'develop' into remoteWipeIterator

8ac4d7d

fdb-hammer options to iterate only over a subset of the specified dom…

70635b5

…ain.

fdb-hammer support for subset iteration in reader mode.

6cccc26

fdb-hammer fixes for subset iteration in itt reader mode.

155a89e

fdb-hammer fix for ITT readers.

6ba1aa3

Merge branch 'develop' into feature/fdb-hammer-consistency-checks

0c5b9ee

fdb-hammer clang formatting.

714bdd6

fdb-hammer replace eckit::Optional by std::optional.

aca2d88

Fixes for consistency checks.

03e5a10

fdb-hammer fix in consistency checks.

6afe3da

Extract config & md checks

2cb1578

Refactor verification checks, and enable simultaneous retrieve and ve…

1b725bf

…rification

Handle data randomisation, full check, and CCSDS causing data sizes t…

92c176e

…o change

Tidy up nodes parsing

10c64f3

Skip field order checks for --X-check with prelisting (order not known)

bd9df13

added check flag (md vs full) to the message body

b98fa39

Merge branch 'feature/refactor-benchmark' into hotfix/5.18.1

eba4a21

format

a86d2a8

version bump (5.18.1)

188ce86

Merge branch 'hotfix/5.18.1'

1f1a8ad

Merge branch 'master' into remoteWipeIterator

8b1c2cd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Wipe refactoring + Toc/Remote Wipe #158

Wipe refactoring + Toc/Remote Wipe #158

danovaro commented Aug 2, 2025 •

edited by github-actions bot

Loading

Uh oh!

simondsmart left a comment

Uh oh!

simondsmart Sep 24, 2025

Uh oh!

simondsmart Sep 24, 2025

Uh oh!

simondsmart Sep 24, 2025

Uh oh!

simondsmart Sep 24, 2025

Uh oh!

simondsmart Sep 24, 2025

Uh oh!

simondsmart Sep 24, 2025

Uh oh!

simondsmart Sep 26, 2025

Uh oh!

simondsmart Sep 26, 2025

Uh oh!

simondsmart Sep 26, 2025

Uh oh!

simondsmart Sep 26, 2025

Uh oh!

simondsmart Sep 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants


		void WipeElement::print(std::ostream& out) const {

		LOG_DEBUG_LIB(LibFdb5) << "Wipe(type=" << type_ << ",msg=" << msg_ << ",uris=[";


		eckit::URI locationURI{index.location().uri()};

		if (!locationURI.path().dirName().sameAs(basePath())) {

Wipe refactoring + Toc/Remote Wipe #158

Are you sure you want to change the base?

Wipe refactoring + Toc/Remote Wipe #158

Conversation

danovaro commented Aug 2, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

danovaro commented Aug 2, 2025 •

edited by github-actions bot

Loading