Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

dongahn
Copy link
Member

@dongahn dongahn commented Mar 4, 2020

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 and rv1_nosched formats.

Fix #610 and #383.

Required by the latest RFC20 (Resource Set Version 1 format).
@garlick
Copy link
Member

garlick commented Mar 4, 2020

One quick comment: some jansson calls that can return errors are not being checked, such as json_array(), json_dumps(), json_object_set_new() , json_object(), json_string().

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-io
Copy link

codecov-io commented Mar 4, 2020

Codecov Report

Merging #613 into master will decrease coverage by 0.74%.
The diff coverage is 67.63%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
resource/utilities/command.cpp 72.18% <ø> (-0.21%) ⬇️
resource/modules/resource_match.cpp 73.33% <ø> (-0.06%) ⬇️
resource/readers/resource_reader_jgf.cpp 69.35% <0%> (ø) ⬆️
resource/traversers/dfu_impl_update.cpp 80.29% <100%> (+0.21%) ⬆️
resource/writers/match_writers.hpp 100% <100%> (ø) ⬆️
resource/writers/match_writers.cpp 72.41% <67%> (-21.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8238aee...0adf5ca. Read the comment docs.

@dongahn
Copy link
Member Author

dongahn commented Mar 4, 2020

One quick comment: some jansson calls that can return errors are not being checked, such as json_array(), json_dumps(), json_object_set_new() , json_object(), json_string().

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., json_dumps() and that embedded json_string(). I will look at it more closely and fix them.

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

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 initialize method to do this resource acquisition, but the ideal pattern would be to do this within constructors. I could actually turn ENOMEM condition into my a custom exceptions within the constructors, but I have generally avoided prevalent use of exceptions.

BTW, this also reminds me that I need to check the error code from the callsite of initialize, which I will do as well.

@dongahn
Copy link
Member Author

dongahn commented Mar 4, 2020

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., json_dumps() and that embedded json_string(). I will look at it more closely and fix them.

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.

dongahn added 4 commits March 4, 2020 11:05
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.
@dongahn
Copy link
Member Author

dongahn commented Mar 4, 2020

I introduced the initialize method to do this resource acquisition, but the ideal pattern would be to do this within constructors. I could actually turn ENOMEM condition into my a custom exceptions within the constructors, but I have generally avoided prevalent use of exceptions.

BTW, this also reminds me that I need to check the error code from the callsite of initialize, which I will do as well.

Now that I think about this, there should be only one call-site of initialize, the factory method for a writer where we do catch the std::bad_alloc exception. So I can have the writer constructors raise this exception when ENOMEM is encountered without having to worry about that exception spilled up the stack.

Let me close this PR and repost with that change. Sorry for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit starttime and expiration from our RV1 emitter
3 participants