DM-53238: Detect recoverable errors in Task SQL execution

[jgates108]

If the SQL statement for a Task fails during execution, the entire user query is cancelled. It may be beneficial to take different actions by taking different actions for different errors. SQL syntax errors are not recoverable, but missing tables could be retried on a different worker.

[jgates108]

Changes in util/Error and util/MultiError are meant to allow the errors from workers to be passed back and merged with other errors from the Executive. Then returned to the user with a reasonable number of lines describing the error (i.e. not have the real probalem burried under thousands of "cancel" messages.)

[jgates108]

This code prevents using a Job to an UberJob to worker where it is known that the table is missing.

The map creation time is important as should make information about missing tables obsolete.

[jgates108]

Send the error numbers back to the czar so the czar can tell if this error was caused by a missing table.

[jgates108]

Examine the error from the worker. Errors caused by missing tables may be recoverable. _unassign the jobs so that buildAndSendUberjobs can try to find other workers to try.

[jgates108]

This just should not have been here. I'm not sure if it was a rebase thing or just wasn't deleted when it was moved.

[iagaponenko]

Is there a limit for the job attempts in the new code? And if so then where this limit is set?

[jgates108]

This was related to an old merging scheme that isn't in use on the branch, and probably isn't even used in the main branch.

The number of job attempts is set from the configuration file and doesn't have an arbitrary limit.

[iagaponenko]

Is there "incomplete" success?

[jgates108]

There could be a partial success, where several jobs succeed and then there was a failure, such as result too large. I'm just trying to indicate that the user query is successful, not just a part of it.

[iagaponenko]

Then perhaps you need something more than just the binary flag, like what's already defined in:

class QInfo {
public:
  
    /**
     *  Constants for query status.
     */
    enum QStatus {
        EXECUTING,  ///< Query is currently executing (or being prepared)
        COMPLETED,  ///< Query execution completed successfully
        FAILED,     ///< Query execution failed
        FAILED_LR,  ///< Query execution failed due to large result set
        ABORTED     ///< Query execution was intentionally aborted
    };

[iagaponenko]

The implementation of the function is too complex. It can be rewritten and reduced to something that is easier to comprehend:

    auto const itr = wContactMap->find(targetWorker->getWorkerId());
    if (itr == wContactMap->end()) return false;
    auto const workerInfo = itr->second;
    return workerInfo == nullptr ||
           jqPtr->isWorkerInAvoidMap(workerInfo, czFamilyMap->getLastUpdateTime());

[iagaponenko]

What is the meaning and effect on the error reporting model of subCode? Could it be documented, please?

[iagaponenko]

Those two tests for

targetWorker == nullptr || targetWorker->isDead()

could be included in the implementation of avoidThisWorker.

[iagaponenko]

I understand what this method does. However, should the result be sorted based on the severity of the errors?

[jgates108]

These errors are presented to the user in the order they are in this map, so putting the most serious ones first is probably helpful.

[iagaponenko]

It's a mistake to assume that std::maps has any "ordering". According to the official documentation on this collection: https://en.cppreference.com/w/cpp/container/map.html

std::map is a sorted associative container that contains key-value pairs with unique keys. Keys are sorted by using the comparison function Compare. Search, removal, and insertion operations have logarithmic complexity. Maps are usually implemented as Red–black trees.

However, when fetching elements from the map:

Iterators of std::map iterate in ascending order of keys, where ascending is defined by the comparison that was used for construction.

What the last statement means is that in the default instantiation (the default comparator std::less<Key>), elements of your vector will be sorted using the following operator:

bool std::pair<int, int>::operator<(...)

Note that this is related to my next comment on your PR regarding the meaning of those two ints:

The structure (the meaning of the integers) of the composite key needs to be explained (commented).

Therefore, to guarantee the "most serious" types first, you would need to implement the comparator and pass the instantiation of the map:

namespace pmr {
    template<
        class Key,
        class T,
        class Compare = std::less<Key>
    > using map = std::map<Key, T, Compare,
                           std::pmr::polymorphic_allocator(<std::pair<const Key, T>>>;
}

You need to define your own class MyCompare and declare the data member as shown below:

class MyCompare { 
    ...
};

std::map<std::pair<int, int>, Error, MyCompare> _errorMap;

[iagaponenko]

Forgot to mention an alternative solution. In case some specific ordering of the vector's elements is neeed, you may sort the vector using the method by:

#include <algorithm>
...
std::vector<Error> result = ...;
std::sort(result.begin(), result.end(), [] (Error const& a, Error const& b) -> bool {
    return a.getCode() < b.getCode();
});
return result;

[jgates108]

std::map<std::pair<int, int>, Error> _errorMap;
Where the pair of ints is code, subCode. Lower values for code are stated to be higher priority. pair and tuple do comparisons in order. Nothing interesting needs to be done to get high priority codes to the front of the list.

[iagaponenko]

The structure (the meaning of the integers) of the composite key needs to be explained (commented).

[iagaponenko]

Should the map be defined as:

std::map<std::pair<Error::ErrCode, int>, Error> _errorMap;

?

[jgates108]

When I see Error::ErrCode ec I really expect any value there to be defined in Error::ErrCode. I know C++ really defines that as int. I see setting ErrCode to something that isn't in ErrCode as a mistake.
int says any integer can be used. In this case, putting all possible error codes in ErrCode just isn't practical.

[iagaponenko]

Ideally, yes, all possible codes would need to be mentione din the enum definition. In reality, C/C++ won't care if enum is define din this way (not as enum class : int). My point was that using the enum type in the relevant code would improve readability. As you know, there may be many "error codes" in Qserv.

The error handling in Qserv is the reall mess!

[iagaponenko]

        errStr += string(" exception:") + e.what();

[iagaponenko]

Excessive construction can make an extra copy. The following would be enough:

string strMsg = "worker run error chunk=" + to_string(_chunkId) + ":" + errStr;

[iagaponenko]

You can do it all in one line:

string const strMsg = "worker run error, chunk=" + to_string(_chunkId) + ":" + errStr + ", worker=" + getUberJobData()->getWorkerId();

[iagaponenko]

Should the return type of the method be enum type ErrCode?

[iagaponenko]

Should the type be:

ErrCode _code = NONE;

?

[iagaponenko]

Should the map be defined as:

std::map<std::pair<Error::ErrCode, int>, Error> _errorMap;

?

[iagaponenko]

Looks okay to me.