-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/gribjump plugin #28
Conversation
NB In case I forget: requires new All tests will fail to build until then. |
@@ -155,6 +158,7 @@ class FDB { | |||
|
|||
FDBStats stats_; | |||
|
|||
FlushCallback flushCallback_ = CALLBACK_FLUSH_NOOP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In constructor:
flushCallback_ = LibFDB::instance().defaultFlushCallback();
src/fdb5/api/FDB.cc
Outdated
} | ||
|
||
void FDB::initPlugins(const Config& config){ | ||
bool enableGribjump = eckit::Resource<bool>("fdbEnableGribjump;$FDB_ENABLE_GRIBJUMP", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not want to see the words gribJump in the FDB (or really even the word GRIB)
@@ -20,8 +20,10 @@ | |||
|
|||
namespace fdb5 { | |||
|
|||
using ArchiveCallback = std::function<void(const Key&, const FieldLocation&)>; | |||
using ArchiveCallback = std::function<void(const Key& key, const void* data, size_t length, const FieldLocation&)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass the data and a promise
Note in comment that the callback must return before the FieldLocation will be satisfied (so promise.get() will need to be called in a thread/async/something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing a future rather than the promise (promise allows you to set the value, future allows you to get the value. Callback should only get the value).
callback must return before the FieldLocation will be satisfied
Personally, I think it would be better if the callback did not have to implemented its own thread (though we do for gribjump): the important thing is that promse.set_value(location) is called before calling the callback (when using a single-threaded fdb backend), or the thread which sets the value has been started (when using remote fdb) before calling the callback.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #28 +/- ##
===========================================
+ Coverage 63.49% 63.52% +0.02%
===========================================
Files 236 236
Lines 13391 13410 +19
Branches 1291 1291
===========================================
+ Hits 8503 8519 +16
- Misses 4888 4891 +3 ☔ View full report in Codecov by Sentry. |
Looks great. Let's do it. |
Changes:
void* data
andlength
FlushCallbacks
.initPlugins
method which is called by FDB constructor, which calls checks if gribjump plugin is enabled and passesthis
to gribjump if so.Questions: