Skip to content

Commit 378cad9

Browse files
jgates108jgates108
committed
Review changes.
1 parent ef0929a commit 378cad9

16 files changed

+39
-49
lines changed

‎src/qdisp/Executive.cc

+3-4
Original file line numberDiff line numberDiff line change
@@ -375,8 +375,7 @@ void Executive::markCompleted(JobId jobId, bool success) {
375375
if (!success && !isRowLimitComplete()) {
376376
{
377377
lock_guard<mutex> lock(_incompleteJobsMutex);
378-
auto iter = _incompleteJobs.find(jobId);
379-
if (iter == _incompleteJobs.end()) {
378+
if (_incompleteJobs.count(jobId) == 0) {
380379
string msg = "Executive::markCompleted failed to find TRACKED " + idStr +
381380
" size=" + to_string(_incompleteJobs.size());
382381
// If the user query has been cancelled, this is expected for jobs that have not yet
@@ -410,7 +409,7 @@ void Executive::markCompleted(JobId jobId, bool success) {
410409
}
411410
_unTrack(jobId);
412411
if (!success && !isRowLimitComplete()) {
413-
squash(string("markComplete error ") + errStr); // ask to squash
412+
squash("markComplete error " + errStr); // ask to squash
414413
}
415414
}
416415

@@ -776,7 +775,7 @@ void Executive::checkResultFileSize(uint64_t fileSize) {
776775
if (total > maxResultTableSizeBytes) {
777776
LOGS(_log, LOG_LVL_ERROR, "Executive: requesting squash, result file size too large " << total);
778777
util::Error err(util::ErrorCode::CZAR_RESULT_TOO_LARGE,
779-
string("Incomplete result already too large ") + to_string(total));
778+
"Incomplete result already too large " + to_string(total));
780779
_multiError.push_back(err);
781780
squash("czar, file too large");
782781
}

‎src/qdisp/Executive.h

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

264-
util::InstanceCount _icEx{"Executive"};
264+
util::InstanceCount const _icEx{"Executive"};
265265
std::atomic<bool> _empty{true};
266266
std::shared_ptr<qmeta::MessageStore> _messageStore; ///< MessageStore for logging
267267

‎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
@@ -230,7 +230,7 @@ class InfileMerger {
230230
bool _applySqlLocal(std::string const& sql, sql::SqlResults& results, sql::SqlErrorObject& errObj);
231231
bool _sqlConnect(sql::SqlErrorObject& errObj);
232232

233-
util::InstanceCount _icIm{"InfileMerger"};
233+
util::InstanceCount const _icIm{"InfileMerger"};
234234
std::string _getQueryIdStr();
235235
void _setQueryIdStr(std::string const& qIdStr);
236236
void _fixupTargetName();

‎src/rproc/ProtoRowBuffer.cc

-2
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,6 @@ ProtoRowBuffer::ProtoRowBuffer(proto::ResponseData const& res, int jobId, std::s
8282
}
8383
}
8484

85-
ProtoRowBuffer::~ProtoRowBuffer() { LOGS(_log, LOG_LVL_TRACE, "~ProtoRowBuffer()"); }
86-
8785
/// Fetch a up to a single row from from the Result message
8886
unsigned ProtoRowBuffer::fetch(char* buffer, unsigned bufLen) {
8987
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
@@ -466,6 +466,7 @@ bool Task::setTaskQueryRunner(wdb::QueryRunner::Ptr const& taskQueryRunner) {
466466
}
467467

468468
void Task::freeTaskQueryRunner(wdb::QueryRunner* tqr) {
469+
// Only free _taskQueryRunner if it's the expected one.
469470
if (_taskQueryRunner.get() == tqr) {
470471
_taskQueryRunner.reset();
471472
} 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)