-
Notifications
You must be signed in to change notification settings - Fork 41
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
Use Jansson to emit JSON-based writers + time keys in RV1 #613
Conversation
Required by the latest RFC20 (Resource Set Version 1 format).
One quick comment: some jansson calls that can return errors are not being checked, such as I guess advantage of a native C++ json library here would be presumably it would throw exceptions instead of returning some value you have to check for... |
Codecov Report
@@ Coverage Diff @@
## master #613 +/- ##
==========================================
- Coverage 76.17% 75.43% -0.75%
==========================================
Files 70 70
Lines 7128 7283 +155
==========================================
+ Hits 5430 5494 +64
- Misses 1698 1789 +91
Continue to review full report at Codecov.
|
Ah. I tried to check errors from these calls as much as possible but apparently there were some blind spots. Thank you for catching that! E.g.,
That would be one advantage or disadvantage depending on how you see it. I have seen a large C++ project that disallows use of exception once and for all because of its various side effects:-) But one advantage of a native C++ json would be that it can allow me to follow the RAII pattern a bit better. class writer {
private:
Json m_out1; // Not a raw pointer or
std::shared_ptr<Json> m_out2; // use a C++ shared pointer or unique pointer
}; I introduced the BTW, this also reminds me that I need to check the error code from the callsite of |
Well, I also found the PR actually includes one interim prototype commit which I shouldn't have included where some of those calls are not error checked at all. Let me remove that commit. Sorry about that. |
Problem: Currently, writers commonly use C++ std::stringstream to emit resource set information regardless of emitting formats: i.e., plain-text formats or JSON-based formats alike. For a JSON-based format, however, this approach is becoming increasingly complex to modify when we change the format. Use Jansson to emit all of the JSON-based resource set information including rlite, jgf, rv1 and rv1_nosched formats. Rework the writer interface design a bit to make the memory management semantics of dealing with Janssen objects clearer. Introduce the "initialize" method in which each class can allocate memory for Jansson objects. Remove the "reset" method as the semantics changed such that writer data gets reset whenever "emit" (or "emit_json") is called. Choose Jansson because this library is very well understood at this point in terms of its side effect and etc: this is the main library being used in both at flux-core and other codes within flux-sched. We have considered other C++-based JSON libraries. However, Jansson is a C library and adds one design constraint. I avoided to allocate Jansson member objects (json_t) because it isn't easy to handle ENOMEM conditions. Hence the introduction of the "initialize" method.
Problem: One of the test output files from resource-query overwrites an existing file. While this should still functionality work, this can hamper debugging if this file needs to be examined at its previous state. Use a new output file name to fix it.
Now that I think about this, there should be only one call-site of Let me close this PR and repost with that change. Sorry for the noise. |
This PR reworks the match writers to use Jansson for emitting JSON-based writers.
Currently, the writers commonly use C++
std::stringstream
to emit resource set information regardless of emitting formats: i.e., plain-text formats or JSON-based formats alike. For a JSON-based format, however, this approach is becoming increasingly more complex to modify when we change the format.Use Jansson to emit all of the JSON-based resource set information including
rlite
,jgf
,rv1
andrv1_nosched
formats.Fix #610 and #383.