Remote fdb #54

Ozaq · 2024-11-26T09:11:52Z

No description provided.

…ture/noDB

…roved concurrency)

src/fdb5/CMakeLists.txt

Ozaq · 2024-11-26T09:41:14Z

src/fdb5/types/TypesRegistry.h


 public: // methods

    TypesRegistry();
+    TypesRegistry(eckit::Stream& s);


needs explicit to prevent unexpected type coversion

Ozaq · 2024-11-26T09:47:40Z

src/fdb5/types/TypesRegistry.cc

+    s >> numTypes;
+    for (size_t i=0; i<numTypes; i++) {
+        s >> name;
+        s >> type;
+        types_[name] = type;
+    }
+}


This should probably throw / or report an error otherwise if the stream cannot be read from any longer, e.g the other side hung up unexpectedly.

Ozaq · 2024-11-26T09:54:04Z

src/fdb5/api/DistFDB.h

@@ -47,6 +47,8 @@ class DistFDB : public FDBBase {

    ListIterator list(const FDBToolRequest& request) override;

+    AxesIterator axesIterator(const FDBToolRequest& request, int level=3) override { NOTIMP; }


I think this warrants an explanation why this FDB type does not allow axes iteration.

src/fdb5/api/FDB.h

Ozaq · 2024-11-26T13:21:19Z

src/fdb5/api/FDB.h

RemoteFDB only supports a subset of the interface and throws a NotImplemented exception that will reach caller code. I think this is a surprising design as the Exception is unexpected (nothing in the interface indicates that behavior) and also not very descriptive. Not implemented can have several meanings, at a bare minimum the exception should be translated into a "NotSupportedForThisDBType" exception. It would be nice to be able to programmatically inspect for capabilities, e.g. have something such has bool supports_wipe().

Ozaq · 2024-11-26T13:27:31Z

src/fdb5/api/RemoteFDB.h

-
-    eckit::DataHandle* dataHandle(const FieldLocation& fieldLocation);
-    eckit::DataHandle* dataHandle(const FieldLocation& fieldLocation, const Key& remapKey);
+    ~RemoteFDB() {}


Needs to be virtual/override

Ozaq · 2024-11-26T13:49:17Z

src/fdb5/api/RemoteFDB.cc

-    }
-};
-}
+        // TODO move the endpoint replacement to the server side ()


Can this be done before merging?

Ozaq · 2024-11-26T14:41:13Z

src/fdb5/api/RemoteFDB.cc

+class FDBEndpoint {
+public:
+    eckit::net::Endpoint fieldLocationEndpoint_;
+    bool localFieldLocation_;

-    // TODO: Abstract this dictionary into a RemoteConfiguration object, which
-    //       understands how to do the negotiation, etc, but uses Value (i.e.
-    //       essentially JSON) over the wire for flexibility.
-    s << availableFunctionality().get();
+    FDBEndpoint(const eckit::net::Endpoint& fieldLocationEndpoint, bool localFieldLocation) :
+        fieldLocationEndpoint_(fieldLocationEndpoint), localFieldLocation_(localFieldLocation) {}
+};


This is unused

Ozaq · 2024-11-26T14:42:52Z

src/fdb5/api/RemoteFDB.cc

        new AsyncIterator([messageQueue, remoteFDB](eckit::Queue<ValueType>& queue) {
-            StoredMessage msg = std::make_pair(remote::MessageHeader{}, eckit::Buffer{0});
+            eckit::Buffer msg{0};
                        while (true) {
                            if (messageQueue->pop(msg) == -1) {
                                break;
                            } else {
-                                MemoryStream s(msg.second);
+                                MemoryStream s(msg);
                                queue.emplace(HelperClass::valueFromStream(s, remoteFDB));
                            }
                        }


Formatting seems broken.

Ozaq · 2024-11-26T14:54:00Z

I think this change also needs a couple of high level system tests, e.g. 2 FDB instances one as local, L. And another one as remote R, where R talks to L.

codecov-commenter · 2024-11-26T20:11:10Z

Codecov Report

Attention: Patch coverage is 19.50839% with 2063 lines in your changes missing coverage. Please review.

Project coverage is 61.24%. Comparing base (c6e8b9e) to head (92e37c7).

Files with missing lines	Patch %	Lines
src/fdb5/remote/server/ServerConnection.cc	0.00%	331 Missing ⚠️
src/fdb5/remote/client/ClientConnection.cc	0.00%	276 Missing ⚠️
src/fdb5/remote/server/CatalogueHandler.cc	0.00%	264 Missing ⚠️
src/fdb5/remote/client/RemoteStore.cc	0.00%	227 Missing ⚠️
src/fdb5/remote/server/StoreHandler.cc	0.00%	149 Missing ⚠️
src/fdb5/api/RemoteFDB.cc	0.00%	128 Missing ⚠️
src/fdb5/remote/client/RemoteCatalogue.cc	0.00%	98 Missing ⚠️
src/fdb5/remote/Connection.cc	0.00%	89 Missing ⚠️
src/fdb5/remote/client/ClientConnectionRouter.cc	0.00%	66 Missing ⚠️
src/fdb5/remote/RemoteFieldLocation.cc	0.00%	44 Missing ⚠️
... and 52 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #54      +/-   ##
===========================================
- Coverage    64.50%   61.24%   -3.27%     
===========================================
  Files          238      264      +26     
  Lines        13844    14924    +1080     
  Branches      1334     1474     +140     
===========================================
+ Hits          8930     9140     +210     
- Misses        4914     5784     +870

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

tbkr

Quite a lot obsolete comment/commented-out code remarks.

There are some comments on changed logic as well, which may slipped through, while writing the code.

tbkr · 2024-11-27T08:26:42Z

src/fdb5/api/helpers/ListIterator.cc

+    if (location_) {
+        if (withLocation) {
+            out << sep << *location_;
+        } else if (withLength) {


Logic changed here:

Now only true if location_ and withLength are both true.

tbkr · 2024-11-27T08:27:27Z

src/fdb5/api/local/ControlVisitor.cc

@@ -27,9 +27,9 @@ ControlVisitor::ControlVisitor(eckit::Queue<ControlElement>& queue,
    identifiers_(identifiers) {}


-bool ControlVisitor::visitDatabase(const Catalogue& catalogue, const Store& store) {
+bool ControlVisitor::visitDatabase(const Catalogue& catalogue) { //, const Store& store) {


Remove the unneeded comments.

tbkr · 2024-11-27T08:28:01Z

src/fdb5/api/local/ControlVisitor.h

@@ -37,7 +37,8 @@ class ControlVisitor : public QueryVisitor<ControlElement> {
    bool visitIndexes() override { return false; }
    bool visitEntries() override { return false; }

-    bool visitDatabase(const Catalogue& catalogue, const Store& store) override;
+    bool visitDatabase(const Catalogue& catalogue) override;
+//    bool visitDatabase(const Catalogue& catalogue, const Store& store) override;


tbkr · 2024-11-27T08:28:24Z

src/fdb5/api/local/DumpVisitor.h

@@ -44,7 +44,8 @@ class DumpVisitor : public QueryVisitor<DumpElement> {
    bool visitIndexes() override { return false; }
    bool visitEntries() override { return false; }

-    bool visitDatabase(const Catalogue& catalogue, const Store& store) override {
+//    bool visitDatabase(const Catalogue& catalogue, const Store& store) override {


tbkr · 2024-11-27T08:28:47Z

src/fdb5/api/local/ListVisitor.h

@@ -43,14 +43,16 @@ struct ListVisitor : public QueryVisitor<ListElement> {

    /// Make a note of the current database. Subtract its key from the current
    /// request so we can test request is used in its entirety
-    bool visitDatabase(const Catalogue& catalogue, const Store& store) override {
+//    bool visitDatabase(const Catalogue& catalogue, const Store& store) override {


Remove, below as well.

tbkr · 2024-11-27T10:41:09Z

src/fdb5/toc/TocPurgeVisitor.h

@@ -32,7 +32,8 @@ class TocPurgeVisitor : public PurgeVisitor, public TocStatsReportVisitor {
    TocPurgeVisitor(const TocCatalogue& catalogue, const Store& store);
    ~TocPurgeVisitor() override;

-    bool visitDatabase(const Catalogue& catalogue, const Store& store) override;
+    // bool visitDatabase(const Catalogue& catalogue, const Store& store) override;


tbkr · 2024-11-27T10:41:20Z

src/fdb5/toc/TocStats.cc

@@ -218,7 +218,8 @@ TocStatsReportVisitor::TocStatsReportVisitor(const TocCatalogue& catalogue, bool

 TocStatsReportVisitor::~TocStatsReportVisitor() {}

-bool TocStatsReportVisitor::visitDatabase(const Catalogue& catalogue, const Store& store) {
+//bool TocStatsReportVisitor::visitDatabase(const Catalogue& catalogue, const Store& store) {


tbkr · 2024-11-27T10:41:26Z

src/fdb5/toc/TocStats.h

@@ -159,7 +159,8 @@ class TocStatsReportVisitor : public virtual StatsReportVisitor {

 private: // methods

-    bool visitDatabase(const Catalogue& catalogue, const Store& store) override;
+    // bool visitDatabase(const Catalogue& catalogue, const Store& store) override;


tbkr · 2024-11-27T10:41:45Z

src/fdb5/toc/TocWipeVisitor.cc

@@ -100,14 +100,16 @@ TocWipeVisitor::TocWipeVisitor(const TocCatalogue& catalogue,
 TocWipeVisitor::~TocWipeVisitor() {}


-bool TocWipeVisitor::visitDatabase(const Catalogue& catalogue, const Store& store) {
+// bool TocWipeVisitor::visitDatabase(const Catalogue& catalogue, const Store& store) {


Remove, also below

tbkr · 2024-11-27T10:41:51Z

src/fdb5/toc/TocWipeVisitor.h

@@ -37,7 +37,8 @@ class TocWipeVisitor : public WipeVisitor {

 private: // methods

-    bool visitDatabase(const Catalogue& catalogue, const Store& store) override;
+    // bool visitDatabase(const Catalogue& catalogue, const Store& store) override;


Ozaq · 2024-11-27T08:49:15Z

src/fdb5/database/Store.h

@@ -47,7 +47,7 @@ class Store {

    virtual std::string type() const = 0;
    virtual bool open() = 0;
-    virtual void flush() = 0;
+    virtual size_t flush() = 0;


This looks innocent at first, but ti me this implies that I now have to check the result here and ensure it is what the calling code expects.

EDIT: Ok I have looked at the use and I think this should be changed back to return void. The use I see is only to add an assert to ensure the flush call to the store matches (in terms of number of flushed locations).

src/fdb5/database/Archiver.cc

Ozaq · 2024-11-27T10:47:56Z

src/fdb5/api/RemoteFDB.cc

-public:
-    ConnectionError(const int);
-    ConnectionError(const int, const eckit::net::Endpoint&);
+    static size_t bufferSize() { return 1024*1024; }


That seems to be a bit excessive, from looking at the mars request logs from one day (2024-11-11) I can see that <1% requests exceed 4Kb (as text) and the largest is ~245Kb. This probably should not be a one size fits all approach. It would be very nice if a MarsRequest type would be able to give an upper bound / exact size for its serialization methods.

IIRC, this was increased from 4096 over the summer when we needed to list/read from 40 years of climate-dt data on the lumi databridge.

Probably would be sensible to have a smaller buffer, and transfer larger requests in chunks if need be.

Ozaq · 2024-11-27T11:10:53Z

src/fdb5/database/EntryVisitMechanism.h

@@ -57,7 +62,7 @@ class EntryVisitor : public eckit::NonCopyable {

    // n.b. non-owning
    const Catalogue* currentCatalogue_ = nullptr;
-    const Store* currentStore_ = nullptr;
+    mutable Store* currentStore_ = nullptr;


Since this now gets deleted, the above comment does not longer fit. Also this should be a unique_ptr as it is even initialized from the release of a unique_ptr in EntryVisitor.cc:46

Fixed comments, created a ticket for the unique_ptr

Ozaq · 2024-11-27T11:11:29Z

src/fdb5/database/EntryVisitMechanism.cc

+Store& EntryVisitor::store() const {
+    if (!currentStore_) {
+        ASSERT(currentCatalogue_);
+        currentStore_ = currentCatalogue_->buildStore().release();


This should not be a raw pointer but a unique_ptr

Ozaq · 2024-11-27T12:39:29Z

src/fdb5/api/RemoteFDB.cc

+    eckit::Buffer buf = controlWriteReadResponse(remote::Message::Stores, generateRequestID());
+    eckit::MemoryStream s(buf);
+    size_t numStores;
+    s >> numStores;
+    ASSERT(numStores > 0);


I don't like that we deserialize from a stream here although we send a specific message on the control channel. I would expect that there is a specific response for each request and hence the response should arrive back at the caller deserialised

E.g.

StoreResponse response = controlWriteReadResponse(remote::Message::Stores, generateRequestID()) if(response.stores == 0) { // and so on

Ozaq · 2024-11-27T12:57:27Z

src/fdb5/api/RemoteFDB.cc

-
-    if (!archiveFuture_.valid()) {
+// Client
+bool RemoteFDB::handle(remote::Message message, bool control, uint32_t requestID) {


The control parameter is unused

Ozaq · 2024-11-27T13:05:35Z

src/fdb5/remote/client/ClientConnection.cc

+                if (hdr.clientID()) {
+                    bool handled = false;
+
+                    ASSERT(hdr.control() || single_);
+
+                    auto pp = promises_.find(hdr.requestID);
+                    if (pp != promises_.end()) {
+                        std::lock_guard<std::mutex> lock(promisesMutex_);
+                        if (hdr.payloadSize == 0) {
+                            ASSERT(hdr.message == Message::Received);
+                            pp->second.set_value(eckit::Buffer(0));
+                        } else {
+                            pp->second.set_value(std::move(payload));
+                        }
+                        promises_.erase(pp);
+                        handled = true;
+                    } else {
+                        Client* client = nullptr;
+                        {
+                            std::lock_guard<std::mutex> lock(clientsMutex_);
+
+                            auto it = clients_.find(hdr.clientID());
+                            if (it == clients_.end()) {
+                                std::stringstream ss;
+                                ss << "ERROR: connection=" << controlEndpoint_ << " received [clientID="<< hdr.clientID() << ",requestID="<< hdr.requestID << ",message=" << hdr.message << ",payload=" << hdr.payloadSize << "]" << std::endl;
+                                ss << "Unexpected answer for clientID recieved (" << hdr.clientID() << "). ABORTING";
+                                eckit::Log::status() << ss.str() << std::endl;
+                                eckit::Log::error() << "Retrieving... " << ss.str() << std::endl;
+                                throw eckit::SeriousBug(ss.str(), Here());


At the end of this code block it is considered to be a serious bug if the client id is unknown, however we only enter this code if there is ANY client ID. If there is no client ID the code will just continue with the next iteration of the for-ever loop. Is this intentional? I would argue that no client id is the same error case as an unknown client id.

src/fdb5/remote/client/RemoteCatalogue.h

Ozaq · 2024-11-27T13:09:27Z

src/fdb5/remote/client/RemoteCatalogue.h

+    void checkUID() const override;
+    eckit::URI uri() const override;
+
+    void sendArchiveData(uint32_t id, const Key& key, std::unique_ptr<FieldLocation> fieldLocation);


This method is unused as far as I can tell.

src/fdb5/remote/client/RemoteCatalogue.cc

Ozaq · 2024-11-27T14:02:42Z

src/fdb5/remote/client/RemoteCatalogue.cc

+    Buffer keyBuffer(4096);
+    MemoryStream keyStream(keyBuffer);
+    keyStream << currentIndexKey_;
+    keyStream << key;
+
+    Buffer locBuffer(4096);
+    MemoryStream locStream(locBuffer);
+    locStream << *fieldLocation;
+
+    std::vector<std::pair<const void*, uint32_t>> payloads;
+    payloads.push_back(std::pair<const void*, uint32_t>{keyBuffer, keyStream.position()});
+    payloads.push_back(std::pair<const void*, uint32_t>{locBuffer, locStream.position()});
+
+    dataWrite(Message::Blob, id, payloads);


There is serialization and deserialization code in many places and I think this needs to be abstracted away into the read/write methods.

With the current implementation I see problems if we ever want to evolve the protocol, either by changing the serialization format or by introducing new / deprecating old fields.

I would like to see something along the lines of:

struct MessageBlob { // Contains everything the message entails }; auto msg = createMessage(...) // Or use a builder write(msg) // This takes care of buffer handling & serialization

…ranch)

… cache + Client handle simplified)

This reverts commit 5a6a08f.

danovaro and others added 30 commits October 6, 2023 14:25

wip

7a593ab

Schema streamable

8fc61a7

wip

1d65ce7

wip

b6691d5

cleanup

337cc51

removed DB, added fdb remote connection multiplexer

d5f9ae8

removed default Schema ctor

563d1ad

fixed RemoteFieldLocation URI parsing + UriStore cleanup

020ce9f

Remote catalogue write pathway wip

e9df4ee

Merge branch 'develop' into feature/noDB

8b4a464

fdb remote - fix flush of multiple DBs

d6a0003

fdb remote - read

cee3a23

Remote catalogue list and api forwarding

618e3da

Merge branch 'feature/noDB' into feature/noDB

4908bc5

receive store config from catalogue

2e8d795

Remote retrieve and archive using control endpoint

6a2843d

Merge branch 'feature/noDB' of ssh://git.ecmwf.int/mars/fdb5 into fea…

40e783f

…ture/noDB

lazy creation of remote::Archiver

6c7c24b

Merge branch 'feature/noDB' of ssh://git.ecmwf.int/mars/fdb5 into fea…

2d6666f

…ture/noDB

list/retrieve/archive with both catalogue and store remote

9894117

random store

eb715c0

removed remote::Client::key()

79e22bf

Merge branch 'master' into feature/noDB

69e2b9d

addressed PR comments (blocking requests are using data channel - imp…

dfb54b4

…roved concurrency)

cleanup

8ab2cc2

wip

e5c17d7

wip

8eee95e

wip

da0a0f7

wip

40fcf66

wip

d46037c

danovaro added 2 commits November 11, 2024 15:46

config remoteFDB connection(s)

8397826

fix merge

872573a

Ozaq commented Nov 26, 2024

View reviewed changes

Merge branch 'develop' into remoteFDB

d0af10f

tbkr requested changes Nov 27, 2024

View reviewed changes

Ozaq commented Nov 27, 2024

View reviewed changes

danovaro and others added 5 commits November 29, 2024 14:16

wip

9057f3e

Base tmpdir on cwd in auxiliary/callback tests

db308cf

FDB-330 lustre FS identification

9d2bebd

FDB-330 lustre FS identification

99d130a

addressed PR + fix toc async read

e5f2ace

Ozaq assigned danovaro Dec 6, 2024

danovaro and others added 5 commits December 6, 2024 14:06

Merge branch 'develop' into remoteFDB

f4245dc

retrieve seekable handle removed (and ported to feature/57-seekable b…

49dcda0

…ranch)

retrieve seekable handle removed (and ported to feature/57-seekable b…

b64b924

…ranch)

retrieve seekable handle removed in the c API

7e03919

Remove commented code

46508f3

Ozaq mentioned this pull request Dec 11, 2024

EntryVisititor should use a unique_ptr<Store> instead of a raw pointer #61

Open

danovaro and others added 9 commits December 11, 2024 17:38

Fix PR

b70c849

Remove commented out code & formatting

43ce247

addressed PR comments (mutex on flush while evicting a catalogue from…

5974d92

… cache + Client handle simplified)

compute Axes encoding size (to allocate the correct buffer size)

598a1d7

added Schema dependency

10d10b0

wip

5a6a08f

Revert "wip"

4ea6a5b

This reverts commit 5a6a08f.

reverted to Store* in EntryVisitor

b07fa9f

version bump

92e37c7

		@@ -47,6 +47,8 @@ class DistFDB : public FDBBase {

		ListIterator list(const FDBToolRequest& request) override;

		AxesIterator axesIterator(const FDBToolRequest& request, int level=3) override { NOTIMP; }

Remote fdb #54

Are you sure you want to change the base?

Remote fdb #54

Conversation

Ozaq commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ozaq commented Nov 26, 2024

codecov-commenter commented Nov 26, 2024 • edited Loading

Codecov Report

tbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ozaq Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 26, 2024 •

edited

Loading

Ozaq Dec 11, 2024 •

edited

Loading