Skip to content

Commit 3cf3fd2

Browse files
jgates108jgates108
committed
Review changes.
1 parent eaad4a1 commit 3cf3fd2

16 files changed

+39
-49
lines changed

‎src/qdisp/Executive.cc

+3-4
Original file line numberDiff line numberDiff line change
@@ -440,8 +440,7 @@ void Executive::markCompleted(JobId jobId, bool success) {
440440
if (!success && !isRowLimitComplete()) {
441441
{
442442
lock_guard<mutex> lock(_incompleteJobsMutex);
443-
auto iter = _incompleteJobs.find(jobId);
444-
if (iter == _incompleteJobs.end()) {
443+
if (_incompleteJobs.count(jobId) == 0) {
445444
string msg = "Executive::markCompleted failed to find TRACKED " + idStr +
446445
" size=" + to_string(_incompleteJobs.size());
447446
// If the user query has been cancelled, this is expected for jobs that have not yet
@@ -475,7 +474,7 @@ void Executive::markCompleted(JobId jobId, bool success) {
475474
}
476475
_unTrack(jobId);
477476
if (!success && !isRowLimitComplete()) {
478-
squash(string("markComplete error ") + errStr); // ask to squash
477+
squash("markComplete error " + errStr); // ask to squash
479478
}
480479
}
481480

@@ -841,7 +840,7 @@ void Executive::checkResultFileSize(uint64_t fileSize) {
841840
if (total > maxResultTableSizeBytes) {
842841
LOGS(_log, LOG_LVL_ERROR, "Executive: requesting squash, result file size too large " << total);
843842
util::Error err(util::ErrorCode::CZAR_RESULT_TOO_LARGE,
844-
string("Incomplete result already too large ") + to_string(total));
843+
"Incomplete result already too large " + to_string(total));
845844
_multiError.push_back(err);
846845
squash("czar, file too large");
847846
}

‎src/qdisp/Executive.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ class Executive : public std::enable_shared_from_this<Executive> {
281281
/// The stats are pushed to qdisp::CzarStats.
282282
void _updateStats() const;
283283

284-
util::InstanceCount _icEx{"Executive"};
284+
util::InstanceCount const _icEx{"Executive"};
285285
std::atomic<bool> _empty{true};
286286
std::shared_ptr<qmeta::MessageStore> _messageStore; ///< MessageStore for logging
287287

‎src/qdisp/JobDescription.cc

-2
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,6 @@ JobDescription::JobDescription(qmeta::CzarId czarId, QueryId qId, JobId jobId, R
5757
_chunkQuerySpec(chunkQuerySpec),
5858
_mock(mock) {}
5959

60-
JobDescription::~JobDescription() { LOGS(_log, LOG_LVL_TRACE, "~JobDescription()"); }
61-
6260
bool JobDescription::incrAttemptCount(std::shared_ptr<Executive> const& exec, bool increase) {
6361
if (increase) {
6462
++_attemptCount;

‎src/qdisp/JobDescription.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class JobDescription {
6969
JobDescription(JobDescription const&) = delete;
7070
JobDescription& operator=(JobDescription const&) = delete;
7171

72-
virtual ~JobDescription();
72+
virtual ~JobDescription() = default;
7373

7474
std::string cName(const char* fnc) { return std::string("JobDescription::") + fnc + " " + _qIdStr; }
7575

‎src/qdisp/JobQuery.cc

+3-4
Original file line numberDiff line numberDiff line change
@@ -63,16 +63,15 @@ bool JobQuery::cancel(bool superfluous) {
6363
VMUTEX_NOT_HELD(_jqMtx);
6464
lock_guard lock(_jqMtx);
6565

66-
ostringstream os;
67-
os << _idStr << " cancel";
68-
LOGS(_log, LOG_LVL_DEBUG, os.str());
66+
string const context = _idStr + " cancel";
67+
LOGS(_log, LOG_LVL_DEBUG, context);
6968
auto exec = _executive.lock();
7069
if (exec == nullptr) {
7170
LOGS(_log, LOG_LVL_ERROR, " can't markComplete cancelled, executive == nullptr");
7271
return false;
7372
}
7473
if (!superfluous) {
75-
exec->addMultiError(-1, os.str(), util::ErrorCode::RESULT_IMPORT);
74+
exec->addMultiError(-1, context, util::ErrorCode::RESULT_IMPORT);
7675
}
7776
exec->markCompleted(getJobId(), false);
7877
return true;

‎src/rproc/InfileMerger.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ class InfileMerger {
170170
bool _applySqlLocal(std::string const& sql, sql::SqlResults& results, sql::SqlErrorObject& errObj);
171171
bool _sqlConnect(sql::SqlErrorObject& errObj);
172172

173-
util::InstanceCount _icIm{"InfileMerger"};
173+
util::InstanceCount const _icIm{"InfileMerger"};
174174
std::string _getQueryIdStr();
175175
void _setQueryIdStr(std::string const& qIdStr);
176176
void _fixupTargetName();

‎src/rproc/ProtoRowBuffer.cc

-2
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,6 @@ ProtoRowBuffer::ProtoRowBuffer(proto::ResponseData const& res)
7777
}
7878
}
7979

80-
ProtoRowBuffer::~ProtoRowBuffer() { LOGS(_log, LOG_LVL_TRACE, "~ProtoRowBuffer()"); }
81-
8280
/// Fetch a up to a single row from from the Result message
8381
unsigned ProtoRowBuffer::fetch(char* buffer, unsigned bufLen) {
8482
unsigned fetched = 0;

‎src/rproc/ProtoRowBuffer.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class ProtoRowBuffer : public mysql::RowBuffer {
3939
ProtoRowBuffer(proto::ResponseData const& res, int jobId, std::string const& jobIdColName,
4040
std::string const& jobIdSqlType, int jobIdMysqlType);
4141

42-
~ProtoRowBuffer() override;
42+
~ProtoRowBuffer() override = default;
4343

4444
unsigned fetch(char* buffer, unsigned bufLen) override;
4545
std::string dump() const override;

‎src/util/Error.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ struct ErrorCode {
7474
*/
7575
class Error {
7676
public:
77-
Error(int code, std::string const& msg = "", int status = ErrorCode::NONE, bool logLvLErr = true);
77+
explicit Error(int code, std::string const& msg = "", int status = ErrorCode::NONE,
78+
bool logLvLErr = true);
7879

7980
Error() = default;
8081
Error(Error const&) = default;
@@ -105,7 +106,7 @@ class Error {
105106

106107
private:
107108
int _code = ErrorCode::NONE;
108-
std::string _msg{""};
109+
std::string _msg;
109110
int _status = ErrorCode::NONE;
110111
};
111112

‎src/util/InstanceCount.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,14 @@ LOG_LOGGER _log = LOG_GET("lsst.qserv.util.InstanceCount");
2121

2222
namespace lsst::qserv::util {
2323

24-
InstanceCountData InstanceCount::_icData;
24+
InstanceCount::InstanceCountData InstanceCount::_icData;
2525

26-
InstanceCountData::InstanceCountData() {
26+
InstanceCount::InstanceCountData::InstanceCountData() {
2727
std::cout << "InstanceCountData " << " mx=" << (void*)(&_mx) << " _inst=" << (void*)(&_instances)
2828
<< " t=" << (void*)(this) << endl;
2929
}
3030

31-
InstanceCountData::~InstanceCountData() {
31+
InstanceCount::InstanceCountData::~InstanceCountData() {
3232
cout << "~InstanceCountData " << " mx=" << (void*)(&_mx) << " _inst=" << (void*)(&_instances)
3333
<< " t=" << (void*)(this) << endl;
3434
}

‎src/util/InstanceCount.h

+15-17
Original file line numberDiff line numberDiff line change
@@ -11,21 +11,6 @@
1111

1212
namespace lsst::qserv::util {
1313

14-
class InstanceCount;
15-
16-
class InstanceCountData {
17-
InstanceCountData();
18-
~InstanceCountData();
19-
20-
friend InstanceCount;
21-
friend std::ostream& operator<<(std::ostream& out, InstanceCount const& instanceCount);
22-
23-
private:
24-
std::map<std::string, int64_t> _instances; //< Map of instances per class name.
25-
std::recursive_mutex _mx; //< Protects _instances.
26-
std::atomic<uint64_t> _instanceLogLimiter{0};
27-
};
28-
2914
/// This a utility class to track the number of instances of any class where it is a member.
3015
//
3116
class InstanceCount {
@@ -39,13 +24,26 @@ class InstanceCount {
3924

4025
int getCount(); //< Return the number of instances of _className.
4126

27+
class InstanceCountData {
28+
InstanceCountData();
29+
~InstanceCountData();
30+
31+
friend InstanceCount;
32+
friend std::ostream& operator<<(std::ostream& out, InstanceCount const& instanceCount);
33+
34+
private:
35+
std::map<std::string, int64_t> _instances; ///< Map of instances per class name.
36+
std::recursive_mutex _mx; ///< Protects _instances.
37+
std::atomic<uint64_t> _instanceLogLimiter{0};
38+
};
39+
4240
friend std::ostream& operator<<(std::ostream& out, InstanceCount const& instanceCount);
4341

4442
private:
43+
void _increment(std::string const& source);
44+
4545
std::string _className; ///< Name of instance being counted.
4646
static InstanceCountData _icData; ///< Map of counts and other data.
47-
48-
void _increment(std::string const& source);
4947
};
5048

5149
} // namespace lsst::qserv::util

‎src/util/MultiError.cc

+3-10
Original file line numberDiff line numberDiff line change
@@ -55,20 +55,13 @@ string MultiError::toOneLineString() const {
5555
int MultiError::firstErrorCode() const { return empty() ? ErrorCode::NONE : _errorVector.front().getCode(); }
5656

5757
string MultiError::firstErrorStr() const {
58+
if (empty()) return string();
5859
ostringstream os;
59-
if (!empty()) {
60-
os << _errorVector.front();
61-
}
60+
os << _errorVector.front();
6261
return os.str();
6362
}
6463

65-
util::Error MultiError::firstError() const {
66-
Error err;
67-
if (!empty()) {
68-
err = _errorVector.front();
69-
}
70-
return err;
71-
}
64+
util::Error MultiError::firstError() const { return empty() ? Error() : _errorVector.front(); }
7265

7366
bool MultiError::empty() const { return _errorVector.empty(); }
7467

‎src/wbase/Task.cc

+1
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,7 @@ bool Task::setTaskQueryRunner(wdb::QueryRunner::Ptr const& taskQueryRunner) {
465465
}
466466

467467
void Task::freeTaskQueryRunner(wdb::QueryRunner* tqr) {
468+
// Only free _taskQueryRunner if it's the expected one.
468469
if (_taskQueryRunner.get() == tqr) {
469470
_taskQueryRunner.reset();
470471
} else {

‎src/wbase/Task.h

+2
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,8 @@ class Task : public util::CommandForThreadPool {
208208
std::string getQueryString() const;
209209
/// Return true if already cancelled.
210210
bool setTaskQueryRunner(std::shared_ptr<wdb::QueryRunner> const& taskQueryRunner);
211+
212+
/// Free this instances TaskQueryRunner object, but only if the pointer matches `tqr`
211213
void freeTaskQueryRunner(wdb::QueryRunner* tqr);
212214
void setTaskScheduler(TaskScheduler::Ptr const& scheduler) { _taskScheduler = scheduler; }
213215
TaskScheduler::Ptr getTaskScheduler() const { return _taskScheduler.lock(); }

‎src/wbase/UserQueryInfo.h

+1
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ class UserQueryInfo {
9494
private:
9595
UserQueryInfo(QueryId qId, CzarIdType czId);
9696

97+
util::InstanceCount const _icUqi{"UserQueryInfo"};
9798
QueryId const _qId; ///< The User Query Id number.
9899
CzarIdType const _czarId;
99100

‎src/wpublish/QueryStatistics.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ class QueryStatistics {
173173
explicit QueryStatistics(QueryId queryId, CzarIdType czarId);
174174
bool _isMostlyDead() const;
175175

176-
util::InstanceCount icqs{"QueryStatistics"};
176+
util::InstanceCount const _icqs{"QueryStatistics"};
177177
mutable std::mutex _qStatsMtx;
178178

179179
std::chrono::system_clock::time_point _touched = std::chrono::system_clock::now();

0 commit comments

Comments
 (0)