tickets/DM-53242

[jgates108]

No description provided.

[iagaponenko]

I have a general comment regarding the design of this class. It looks like this is just a hererogenous container for a bunch of data members. This is AKA the PDO (Plain Data Object). In this case, it may be unreasonable to maintain the complex interface that requires passing a bunch of values to the class's constructor and the factory method create. My recommendation is:

• make the data members public
• eliminate the constructor and allow filling in the data members directly by the client (note that you already have a special method for populating the data members from the JSON object. This method should stay.)
• eliminate the static factory method create()

The second problem with the current design is that you have to pass 11 variables to the factory method and then to the constructor.

Problem number 3 is that both the factorial method (with 11 parameters) and the constructor (with 11 parameters) are public. Normally, if you want to deal with the shared pointers to objects of the class you would want to make the constructor private. Though I think it has to be eliminated as I suggested at the very beginning of this message.

[jgates108]

Edit: Commenting on multiple classes at once,

For UberJobStatusMsg:
Nearly all of the data members are const, and therefore must be set in a constructor.
The constructor is protected.
This is the base class for 2 other classes, which need the constructor, so the constructor cannot be private.
I agree the style needs to be fixed, and since all members are constant, making them public is sensible.

For UberJobReadyMsg:
const members need to be set in constructor, but probably should be public.
Constructor should be private.
There are 11 members because that's the information that needs to be put in the message. The sole purpose of this class is to encode and decode a fairly complicated message.

[iagaponenko]

A constructor with 11 arguments is too heavy to read (by anyone trying to understand and validate your code), it's hard to use, and it's easy to make mistakes (swap strings or numbers). One solution to this problem would be to identify related parameters and group them into a simple PDO. Here is a good candidate:

struct AuthContext {
    std::string instanceId;
    std::string authKey;
    std::string adminAuthKey;
};

Then you can pass this object around by value:

UberJobReadyMsg const msg(AuthContext const& authContext, ...);

Moreover, you can get a value of AuthContext from some configuration service (worker/czar config), etc.

As far as the const-ness of the public data members of some POD is concerned, there are other ways to make them all const by:

UberJobReadyMsg const msg;

Or, if you have the shared pointer:

std::shared_ptr<UberJobReadyMsg const> ptr = ...;

This is up to you. I'm not insisting on my suggestions. My point is that in C++ not everything needs to be a full-blown class. Simple PDOs are often more adequate solutions. The full-blown classes are mostly needed when objects have a mutable/complex state and non-trivial behavior (methods), especially of the state is modified/used by mulriple threads.

[jgates108]

The const has a big advantage in preventing accidental modification anywhere in the code.
Having everything defined in the constructor means that missing a value will cause a compilation error.

AuthContext method look like a good way to reduce the number of parameters in the constructor without losing the other benefits.

[iagaponenko]

It looks like you pushed those members that start with the underscore (_uberJobId, etc.) into the base class by making them protected? In this case, you should not use the leading underscores in the names of the members. Please, see the LSST C++ Style Guide: https://developer.lsst.io/v/DM-6639/coding/cpp_style_guide.html#names-representing-methods-or-functions-should-naturally-describe-the-action-of-the-method-and-so-will-usually-start-with-a-verb-and-must-be-written-in-mixed-case-starting-with-lowercase-private-member-function-names-must-additionally-lead-with-an-underscore

[jgates108]

I'm not particular happy with that aspect of the style guide as changing scope makes it very easy to shadow variables, so I delayed a bit too long in making those changes.

[iagaponenko]

You still have plenty of time to make everything right.

[jgates108]

They are among the things I've been fixing today...

[iagaponenko]

The names of the protected members should not begin with underscores. This is a requirement of the LSST DM C++ Style Guide. Only the private members can.

I would also make these members public and significantly simplify the design of this class and its dependencies by allowing a client to read/write the data members directly. By design, this is just the PDO (Plain Data Object), nothing more.

[iagaponenko]

A comment regarding returning shared_ptr. That's fine if the rest of the code expects and benefits from shared ownership. If callers don't need shared_ptr, returning JSON by value can be simpler and often cheaper due to move semantics.

[iagaponenko]

You don't have to name this function differently. C++ allows selective overloading if you define both methods as:

virtual std::ostream& dump(std::ostream& os) const;
std::string dump() const;

[iagaponenko]

It's simple to return a value rather than the shared pointer:

json UberJobReadyMsg::toJsonPtr() const {
    return json::object({
        {"fileUrl", fileUrl},
        {"rowCount", _rowCount},
        {"fileSize", _fileSize;}
    });
 }

There won't be any performance penalty due to the value (by move) return optimization

[jgates108]

The original idea was to just store copies of the json messages and resend them. It got messy, so I dropped it, but using pointers allowed me to send the message and keep the same message in the map without extra copies. Removing the pointers does make the code simpler.

[iagaponenko]

LGTM

[fritzm]

LGTM -- thanks!