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

Tickets/dm 45548 #884

Draft
wants to merge 22 commits into
base: tickets/DM-43715
Choose a base branch
from
Draft

Tickets/dm 45548 #884

wants to merge 22 commits into from

Conversation

jgates108
Copy link
Contributor

No description provided.

@jgates108 jgates108 marked this pull request as draft January 8, 2025 20:16
@@ -233,6 +233,11 @@ std::string UserQuerySelect::getResultQuery() const {

/// Begin running on all chunks added so far.
void UserQuerySelect::submit() {
auto exec = _executive;
if (exec == nullptr) {
LOGS(_log, LOG_LVL_ERROR, "UserQuerySelect::submit() executive is null at start");
Copy link
Contributor

@iagaponenko iagaponenko Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit confusing. Shouldn't the user query object be constructed with a valid pointer to the "executive"? And if so then why is this test? What makes the pointer to suddenly disappear?
I'm also seeing the very same pattern of checking for the validity of a pointer all around the code of the PR. Perhaps there is a safer way of constructing objects to avoid polutting the code with these tests?
My other worry is that if there is such an uncertainty (with the elusive pointer) then such code must be really hard to debug.

Copy link
Contributor Author

@jgates108 jgates108 Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_executive being null at this point should really never happen, so the check can be removed, but the check is harmless and consistent with other UserQuerySelect methods. UserQuerySelect::discard() sets _executive to null (this has been around for a very long time) so the _executive pointer isn't safe to use directly and must be copied. Changing discard() to not reset _executive had side effects, so I'd rather not mess with it at this time.

@iagaponenko
Copy link
Contributor

iagaponenko commented Jan 14, 2025

I have one general comment on redefining the types of the Standard Library. I think it is not a very good idea to introduce an extra layer of indirection for the pure (where no custom template parameters are involved) std:: types. I would argue that this should NOT be done in the C++ headers due to expanded/exploded visibility of such definitions (It's probably fine to redefine types in a limited scope of the CC files. Though, this still should be done with care after assessing pros-vs-cons of such redefinition). Firstly, such a redirection complicates code analysis (by reviewers), Secondly, it blurs a separation between the Standard types and the custom types. Quite often it's hard to tell what type it is - if it's a user-defined type or the Standard one. So, the reviewer has to figure out where the type definition is made, go to the corresponding header, and memorize that type. My understanding is that a motivation for such a redefinition is probably to "save typing characters" or "see shorted lines" in the code. However, that benefit is rarely achieved when the headers are affected. Here is one example that I have found in the PR.

Consider redirections for Clock and TimeType made in src/qmeta/JobStatus.h:

class JobStatus {
public:
    typedef std::shared_ptr<JobStatus> Ptr;
    using Clock = std::chrono::system_clock;
    using TimeType = std::chrono::time_point<Clock>;
    ...

One can quickly see the first problem here in a way TimeType is named - it is not the time point. This changes expectations of the new type since time type is a very generic term (vs very specific time point of the Standard Library).

Now, let's look at where some of these redefined types are used in src/ccontrol/UserQueryAsyncResult.cc:

int64_t timestampMilli = ...;
qmeta::JobStatus::Clock::duration duration = std::chrono::milliseconds(timestampMilli);
qmeta::JobStatus::TimeType timestamp(duration);

This code is totally confusing. It causes a reviewer to look into the header where both types are defined. Compare this with the clean code w/o type redefinition:

int64_t timestampMilli = ...;
using namespace std;
chrono::system_clock::duration duration = chrono::milliseconds(timestampMilli);
chrono::time_point<chrono::system_clock> timestamp(duration);

That (second version of the) code is very easy to read (to someone who knows what the std::chrono is). That code doesn't create an extra "cognitive barrier" for understanding the original intent of a developer ... which is to make a timestamp in the system clock.

Moreover, your next step in streamlining and improving the code would be to put those 2 lines into a function:

chrono::time_point<chrono::system_clock> systemTimeStamp(int64_t timestampMilli) {
    chrono::system_clock::duration duration = chrono::milliseconds(timestampMilli);
    return chrono::time_point<chrono::system_clock>(duration);
}

And since the function returns the final type then the recipient code would be reduced to use auto as shown below:

int64_t timestampMilli = ...;
auto timestamp = systemTimeStamp(timestampMilli);

That code is very easy to read and understand.

The bottom line is that when you redefine Standard types then you in essence introduce your own (non-standard) vocabulary. This adds extra complexity to the code on top of your application-specific complexity and vocabulary. It's better not to do this if possible.

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.

3 participants