tickets/DM-51870

[jgates108]

No description provided.

[iagaponenko]

This looks fine to me. I have posted a few comments/questions. One thing that is puzzling is the amount of changes made to the implementation of Qserv at both Czar and workers beyond just fixing the unit tests as it's stated in the JIRA ticket description. These (core) changes were not explained or documented in the JIRA ticket.

[iagaponenko]

I'm not seeing any use of the variable in this module.

[jgates108]

Good catch

[iagaponenko]

Did you forget to address this conflict?

[jgates108]

This really confuses me as to how it got here. It wasn't there when I went looking for it. With it, code won't compile, integration tests won't run, etc. Reformating can sneek through as everything else works, but this should have been fatal.

[fritzm]

Yeah, I had the same thought! Yet github reports a successful build and integration tests. Is this file not actually compiled due to a boffed cmake merge maybe? Probably we need to dig in the build log figure this out...

[iagaponenko]

Since you already have a complete JSON object to be added to the collection then it should be push_back. Method emplace_back is used for constructing new objects during insert. Here is an example:

// When to use push_back
std::vector<std::string> v;
std::string s = "hello";
v.push_back(s);       // Copies 's' into the vector
v.push_back("world"); // Constructs a temporary string "world", then moves it into the vector

// When to use emplace_back
std::vector<std::string> v;
v.emplace_back("hello"); // Constructs a string "hello" directly in the vector
v.emplace_back(5, 'c');  // Constructs a string "ccccc" directly in the vector

Method std::vector::emplace_back is conceptually like std::make_shared. For example:

struct Point {
    Point(double x, double y, double z);
};

// One can construct a pointer like this:
auto ptr = std::shared_ptr<Point>(new Point(1, 2, 3));

// Or using:
auto ptr = std::make_shared<Point>(1, 2, 3);

The second method invokes the constructor of Point and passes 3 parameters to the constructor.

[jgates108]

That's all true, and I changed it to push_back, but as far as I know, using push_back is no more efficient than emplace_back in this case. After a little looking around, the it seems push_back may compile slightly faster than emplace_back. Otherwise, if there is a constructor that can build the object directly on container, emplace_back will be faster. So, there doesn't seem to be any real harm using emplace_back in this case, or am I missing something?

[jgates108]

This should construct a new json object directly in the jsqFrags json object thanks to RVO.
jsqFrags.emplace_back(jFrag->toJson());
while this may a new object and then move it.
jsqFrags.push_back(jFrag->toJson());

emplace_back should really be a bit faster.

[iagaponenko]

Performance-wise, and in this particular case, both methods should be the same. My comment was mostly about the semantic differences of the operations. I'm not really insisting on any changes here.

[iagaponenko]

This way of testing the payload of a JSON object is very risky. We should not expect the JSON's dump() method to always produce the same result, or expect that the keys will be printed in the same order. The implementation of the nlohmann library may change in future versions. The safest technique is to check each key of the JSON object:

json obj = ...;
BOOST_REQUIRE(obj.has("maxtablesizemb"));
BOOST_EQUAL(obj.has("maxtablesizemb"), 5432);
...

You may disregard my comment in case if you're using this string for constructing a new JSON object and using that object later for testing. I am just not seeing any use for this method in the change list of this uint test.

[jgates108]

The resulting json objects are compared, not the strings.

[iagaponenko]

Same comment/question as for the method testA()

[iagaponenko]

What is the actual value in adding these 3 methods above? They just print "util::Command" and nothing else.

[jgates108]

I only need to add std::ostream& Command::dump(std::ostream& os) const override to child classes (Task is a child of util::Command) and then the correct version of dump(std::ostream&) const will be called if I do cout << *task; or string str(task->dump());. There's not really any useful info to print for the basic Command class, but providing some output is probably better than nothing.

[iagaponenko]

Is this class Command useful (instantiated) on its own? If not, then perhaps the method should be made pure virtual:

    virtual std::ostream& dump(std::ostream& os) const = 0;

[jgates108]

Then you wouldn't be able to just have a basic util::Command, which is used. These are just for debugging/logging.

[fritzm]

Please squash these 5 commits to one before merging?

(Rationale: The duplicated commit messages with ones already on the history can only be a source of confusion, and are actually "lies" wrt. the content; the way the rebase has sliced the residual changes across multiple commits makes it hard to see the big picture, and doing this will also simplify any future/ongoing rebases to keep up with main.)

[fritzm]

Looks good to me -- thanks!