U/jgates/loader #426

jgates108 · 2018-11-01T19:36:18Z

No description provided.

n8pease · 2018-11-07T21:18:46Z

core/modules/loader/BufferUdp.h

+
+    /// If the buffer already contains valid data, advanceWriteCursor()
+    /// must be called with the length of the valid data. Otherwise,
+    /// BufferUdp thinks it has an empty buffer.


this seems hostile to the user. Why not call the function inside the body of this ctor?

n8pease · 2018-11-07T21:26:31Z

core/modules/loader/BufferUdp.h

+    void advanceWriteCursor(size_t len) {
+        _wCursor += len;
+        if (not isAppendSafe(0)) {
+            throw new std::overflow_error("BufferUdp advanceWriteCursor beyond buffer len=" +


why are you throwing an exception pointer instead of an exception instance? AFAIK this is not conventional.

n8pease · 2018-11-07T21:29:05Z

core/modules/loader/BufferUdp.h

+        _rCursor += len;
+        if (_rCursor > _end) {
+            throw new std::overflow_error("BufferUdp advanceReadCursor beyond buffer len=" +
+                                          std::to_string(len));


do you want to be sure the read cursor does not advance to or beyond the write cursor?

If everything has been passed properly to asio, it shouldn't go outside the buffer. But if it has, this lets us know.

n8pease · 2018-11-07T22:22:23Z

core/modules/loader/BufferUdp.cc

+        // EOF is only a problem if no MsgElement was retrieved.
+        // ??? This is definitely the case in UDP, EOF as nothing more will show up.
+        // ??? But TCP is another issue as EOF is returned when the connection is still live but
+        // ??? there's no data (len=0). Why does read_some set error to eof before the tcp connection is closed?


yeah; according to the docs eof is because the peer closed the connection. You're sure the peer didn't close it?

I'm pretty sure I've seen the warning below show up in the log, but it has been a little while since I went looking for it.

n8pease · 2018-11-07T22:46:28Z

core/modules/loader/BufferUdp.h

+
+/// A buffer for reading and writing. Nothing can be read from the buffer until
+/// something has written to it.
+class BufferUdp {


are there unit tests for this class? it seems like it would be valuable to have some.

There are test for UdpBuffer at the beginning of udpTest.cc. I think there's some room for discussion for the best way to setup unit tests for this.

You mentioned in that file that it's an integration test. There is a lot of code here without testing at the 'unit' granularity, which I think is an issue.

I made a separate ticket for creating unit tests DM-16649

n8pease · 2018-11-07T23:11:44Z

core/modules/loader/Central.h

+    }
+
+
+    virtual std::string getOurLogId() { return "baseclass"; }


maybe an explanation of the name "baseclass" could go here?

And the method needs to be declared const

This should really be removed. Digging through the log was getting confusing as everything wound up in the same log and it was sometimes difficult to discern what class or object wrote the log statement.

n8pease · 2018-11-07T23:13:54Z

core/modules/loader/Central.h

+    boost::asio::io_service& _ioService;
+
+    /// Initialization order is important.
+    DoList _doList{*this}; ///< List of items to be checked at regular intervals.


passing this to another object during initialization is dangerous.

100% agree with that!
Especially after reading the comment above: Initialization order is important.

One alternative would be to dynamically create this object from the body of the class's c-tor after finishing all implicit & explicit initializations of the class's members. The other one would be to extend class DoList with a special setCentral(Central*) method for late binding?

What is there is safe, since the this pointer passed to doList wont be used until the last member '_checkDoListThread' is defined. This is somewhat fragile in that moving the _checkDoListThread declaration can cause a race condition.

The alternative is to break construction into two parts. The regular constructor is called and then a second step, I'll call go(), is called, which sets this pointer and creates the thread and moves in into _checkDoListThread. I'm ok with either. Factory functions don't really work well with base classes, since the child constructors wont call them.

n8pease · 2018-11-07T23:19:28Z

core/modules/loader/Central.h

+
+    std::atomic<uint64_t> _sequence{1};
+
+    util::CommandQueue::Ptr _queue{new util::CommandQueue()};


prefer make_shared to new

n8pease · 2018-11-07T23:21:27Z

core/modules/loader/Central.h

+    std::atomic<uint64_t> _sequence{1};
+
+    util::CommandQueue::Ptr _queue{new util::CommandQueue()};
+    util::ThreadPool::Ptr _pool{util::ThreadPool::newThreadPool(10, _queue)};


it's easy to miss that _queue is new'ed in braces above, and then used in the braces init here, and if the order of these 2 lines gets reversed that could lead to crashes. A comment when doing this kind of thing can be helpful.

Why the "magic number" 10? Could this be put into some configuration service external to teh class Central or be passed into the constructor of class Central?

It goes back to the initialization order being important comment. The choice being between construction order being important, but hidden in the base class. Or, having the complexity of multistage construction that is passed to all the child classes and cannot be hidden.

n8pease · 2018-11-07T23:26:03Z

core/modules/loader/Central.cc

+    while(_loop) {
+        // Run and then sleep for a second. A more advanced timer should be used
+        _doList.checkList();
+        usleep(100000);


why not use a condition variable instead of a timer?

Because it isn't really waiting for anything to happen. The sleep is mostly there to keep this from wasting CPU cycles if _doList is empty, and most of the items on the list have waits (from rate limiting) far longer that 0.1 seconds, so going through the list can easily result in no DoList items actually doing anything.

but there should be a way a member of the list can trigger the condition variable so when there is work to do the loop runs.

The loop always run except for the brief period of time it sleeps (0.1 seconds). Most of the items on the list create a command that is then run in the thread pool. The list itself is not low latency, but the commands that are run by the list can be low latency. All the DoListItems just create commands and add them to the pool, which is pretty quick. Synchronization exists within the running commands, so they can be responsive. The DoList itself does not need to be responsive.

n8pease · 2018-11-07T23:28:04Z

core/modules/loader/DoList.h

+    std::mutex _listMtx; ///< Protects _list (lock this one first)
+
+    std::list<DoListItem::Ptr> _addList;
+    std::mutex _addListMtx; ///< Protects _addList (lock this one second)


thank you for the comments and the narrow scope of the mutex. Big applause!

n8pease · 2018-11-07T23:33:09Z

core/modules/loader/DoList.h

+public:
+    using Ptr = std::shared_ptr<DoListItem>;
+
+    DoListItem() = default;


Igor recommended to me a pattern that I found to be useful when using enable_shared_from_this: make the constructors private and write a static factory function makeNewDoListItem() that returns a populated DoListItem::Ptr. This prevents anyone from creating a DoListItem that is not owned by a shared_ptr, and since calling shared_from_this on a non-pointer-owned-object leads to a misleading error ("bad weak_ptr" if I remember correctly), this prevents you from having to debug that issue & captures it at compile time.

I have used that technique and I like it. I think making the constructor private causes problems with child classes, so forcing the use of a factory function becomes tricky. Maybe protected would work, but the child classes would also need factories.

n8pease · 2018-11-07T23:48:34Z

core/modules/loader/DoList.cc

+        _list.splice(_list.end(), _addList);
+    }
+    for (auto iter = _list.begin(); iter != _list.end(); ++iter){
+        DoListItem::Ptr const& item = *iter;


you can replace both these lines with
for (auto&& item : _list) {

(item will be a DoListItem::Ptr)

I think it's going to be DoListItem::Ptr&

I think I need the iterator to erase elements without having to do a second lookup.

n8pease · 2018-11-07T23:50:53Z

core/modules/loader/DoList.cc

+            LOGS(_log, LOG_LVL_DEBUG, "queuing command");
+            _central.queueCmd(cmd);
+        } else {
+            if (item->removeFromList()) {


is removeFromList really more like shouldRemoveFromList?

as it is, the name reads like a command to me. But from its definition and the usage here, I think it's actually a question...?

iagaponenko

Comments for class BufferUdp.

iagaponenko · 2018-11-07T21:13:52Z

core/modules/loader/ServerUdpBase.h

+    boost::asio::io_service& _ioService;
+    boost::asio::ip::udp::socket _socket;
+    boost::asio::ip::udp::endpoint _senderEndpoint;
+    // char _data[MAX_MSG_SIZE];


Is this still needed?

iagaponenko · 2018-11-07T21:15:46Z

core/modules/loader/BufferUdp.cc

+// Class header
+#include "BufferUdp.h"
+
+// system headers


This comment needs to be delete because no system headers are explicitly included here

iagaponenko · 2018-11-07T21:16:04Z

core/modules/loader/BufferUdp.cc

+
+
+// Third-party headers
+


This comment needs to be delete because no third-party headers are explicitly included here

iagaponenko · 2018-11-07T21:18:16Z

core/modules/loader/BufferUdp.h

+
+/// The absolute largest UDP message we would send.
+/// Usually, they should be much smaller.
+#define MAX_MSG_SIZE 6000


I've looked through the code, and I'm not seeing a case requiring this to be defined as a macro. Perhaps it could be moved as a static data members of class BufferUdp?

iagaponenko · 2018-11-07T21:19:06Z

core/modules/loader/BufferUdp.h

+#include <arpa/inet.h>
+#include <cstring>
+#include <stdexcept>
+#include <memory>


This header needs to be moved one line up

iagaponenko · 2018-11-08T00:28:39Z

core/modules/loader/BufferUdp.cc

+namespace qserv {
+namespace loader {
+
+/// Repeatedly read a socket until a valid MsgElement is read, eof,  or an error occurs.


Normally this comment is expected in the header file.

iagaponenko · 2018-11-08T00:29:02Z

core/modules/loader/BufferUdp.cc

+ * see <http://www.lsstcorp.org/LegalNotices/>.
+ */
+
+


Extra line?

iagaponenko · 2018-11-08T00:29:23Z

core/modules/loader/BufferUdp.cc

+// qserv headers
+#include "loader/LoaderMsg.h"
+
+


Extra line?

iagaponenko · 2018-11-08T00:29:29Z

core/modules/loader/BufferUdp.cc

+// LSST headers
+#include "lsst/log/Log.h"
+
+


Extra line?

core/modules/loader/BufferUdp.cc

iagaponenko

Finished reviewing class Central

iagaponenko · 2018-11-08T02:08:16Z

core/modules/loader/Central.h

+
+// system headers
+#include <boost/bind.hpp>
+#include <boost/asio.hpp>


Boost headers belong to a dedicated group of:

// The third-party headers

iagaponenko · 2018-11-08T02:32:40Z

core/modules/loader/Central.h

+#include "util/ThreadPool.h"
+
+
+


iagaponenko · 2018-11-08T02:35:15Z

core/modules/loader/Central.h

+    ChunkSubchunk(int chunk_, int subchunk_) : chunk(chunk_), subchunk(subchunk_) {}
+    int const chunk;
+    int const subchunk;
+    friend std::ostream& operator<<(std::ostream& os, ChunkSubchunk csc);


Three problems with this operator:

it doe not need to be declared as friend because both data members of struct ChunkSubchunk ar public.

its second parameter needs to be declared as ChunkSubchunk const& csc

the operator needs to be moved out of the struct, probably right after the declaration of the struct

I think the friend designation is harmless and the definition location helps associate the function with the struct.

iagaponenko · 2018-11-08T02:37:02Z

core/modules/loader/Central.h

+    boost::asio::io_service& _ioService;
+
+    /// Initialization order is important.
+    DoList _doList{*this}; ///< List of items to be checked at regular intervals.


100% agree with that!
Especially after reading the comment above: Initialization order is important.

One alternative would be to dynamically create this object from the body of the class's c-tor after finishing all implicit & explicit initializations of the class's members. The other one would be to extend class DoList with a special setCentral(Central*) method for late binding?

iagaponenko · 2018-11-08T02:50:43Z

core/modules/loader/Central.h

+    std::atomic<uint64_t> _sequence{1};
+
+    util::CommandQueue::Ptr _queue{new util::CommandQueue()};
+    util::ThreadPool::Ptr _pool{util::ThreadPool::newThreadPool(10, _queue)};


Why the "magic number" 10? Could this be put into some configuration service external to teh class Central or be passed into the constructor of class Central?

iagaponenko · 2018-11-08T03:25:49Z

core/modules/loader/Central.cc

+
+
+namespace {
+LOG_LOGGER _log = LOG_GET("lsst.qserv.loader.Central");


LSST Logger is not used by the implementation of the class

iagaponenko · 2018-11-08T03:27:48Z

core/modules/loader/Central.h

+#include "loader/MasterServer.h"
+#include "loader/MWorkerList.h"
+#include "loader/WWorkerList.h"
+#include "loader/WorkerServer.h"


This header needs to be moved one line up

iagaponenko · 2018-11-08T03:29:24Z

core/modules/loader/Central.h

+
+    void run();
+
+    std::string getMasterHostName() const { return _masterAddr.ip; }


Should tis method return std::string const& instead?

iagaponenko · 2018-11-08T03:33:43Z

core/modules/loader/Central.h

+    }
+
+
+    virtual std::string getOurLogId() { return "baseclass"; }


And the method needs to be declared const

iagaponenko · 2018-11-08T03:35:51Z

core/modules/loader/Central.h

+
+protected:
+    /// Repeatedly check the items on the _doList.
+    void _checkDoList();


Why tis method seats in the protected area instead of the private one? My understanding is that a thread created in the class's constructor is the only client of the method.

And, by the way, members which are declared as protected should NOT begin with symbol _ (as per the LSST Style Guide).

" protected should NOT begin with symbol _" bleh, so much for being able to easily discern member variables in class functions.

iagaponenko

Reviewed class CentralClient

iagaponenko · 2018-11-08T03:46:00Z

core/modules/loader/CentralClient.h

+ * You should have received a copy of the LSST License Statement and
+ * the GNU General Public License along with this program.  If not,
+ * see <http://www.lsstcorp.org/LegalNotices/>.
+ *


iagaponenko · 2018-11-08T03:46:51Z

core/modules/loader/CentralClient.h

+
+// system headers
+#include <boost/bind.hpp>
+#include <boost/asio.hpp>


Boost headers qualify as the "Third-party headers".
Also, this header needs to be "promoted" one line up.

iagaponenko · 2018-11-08T03:47:37Z

core/modules/loader/CentralClient.h

+namespace qserv {
+namespace loader {
+
+class KeyInfoData : public util::Tracker {


Please, document this class. It's role, contexts in which it's used and how it's going to be used.

iagaponenko · 2018-11-08T03:48:51Z

core/modules/loader/CentralClient.h

+    int subchunk;
+    bool success{false};
+
+    friend std::ostream& operator<<(std::ostream& os, KeyInfoData const& data);


Does not to be friend because all members of the class are public. Besides, the operator may be moved outside the class.

iagaponenko · 2018-11-08T03:49:14Z

core/modules/loader/CentralClient.h

+};
+
+/// TODO Maybe base this one CentralWorker or have a common base class?
+class CentralClient : public Central {


Please, document this class.

iagaponenko · 2018-11-08T06:52:57Z

core/modules/loader/CentralClient.cc

+void CentralClient::handleKeyInsertComplete(LoaderMsg const& inMsg, BufferUdp::Ptr const& data) {
+    LOGS(_log, LOG_LVL_DEBUG, "CentralClient::handleKeyInsertComplete");
+
+    StringElement::Ptr sData = std::dynamic_pointer_cast<StringElement>(MsgElement::retrieve(*data));


core/modules/loader/CentralClient.cc

iagaponenko · 2018-11-08T06:58:16Z

core/modules/loader/CentralClient.cc

+    // Insert a oneShot DoListItem to keep trying to add the key until
+    // we get word that it has been added successfully.
+    LOGS(_log, LOG_LVL_INFO, "Trying to insert key=" << key << " chunk=" << chunk <<
+                             " subchunk=" <<subchunk);


Missing space after << ?

... <<subchunk);

iagaponenko · 2018-11-08T07:01:06Z

core/modules/loader/CentralClient.cc

+/// Returns a pointer to a Tracker object that can be used to track job
+//  completion and the status of the job. keyInsertOneShot will call
+//  _keyInsertReq until it knows the task was completed, via a call
+//  to _handleKeyInsertComplete


The method documentation needs to be in the header file.

iagaponenko

Reviewed class CentralMaster

iagaponenko · 2018-11-08T07:03:34Z

core/modules/loader/CentralMaster.h

+ * see <http://www.lsstcorp.org/LegalNotices/>.
+ *
+ */
+#ifndef LSST_QSERV_LOADER_CENTRALMASTER_H_


Extra _ in the end of the macro definition. It should be:

LSST_QSERV_LOADER_CENTRALMASTER_H

iagaponenko · 2018-11-08T07:03:43Z

core/modules/loader/CentralMaster.h

+ * You should have received a copy of the LSST License Statement and
+ * the GNU General Public License along with this program.  If not,
+ * see <http://www.lsstcorp.org/LegalNotices/>.
+ *


iagaponenko · 2018-11-08T07:04:05Z

core/modules/loader/CentralMaster.h

+
+// system headers
+#include <boost/bind.hpp>
+#include <boost/asio.hpp>


Promote this header one line up

iagaponenko · 2018-11-08T07:04:34Z

core/modules/loader/CentralMaster.h

+#define LSST_QSERV_LOADER_CENTRALMASTER_H_
+
+// system headers
+#include <boost/bind.hpp>


Boost headers are the "Third party" headers, not the "system" ones

iagaponenko · 2018-11-08T07:04:51Z

core/modules/loader/CentralMaster.h

+
+// Qserv headers
+#include "loader/Central.h"
+


iagaponenko · 2018-11-08T07:17:23Z

core/modules/loader/CentralMaster.cc

+                throw LoaderMsgErr(funcName + " Multiple rightMost workers " +
+                        " name=" + std::to_string(rightMostName) +
+                        " name=" + std::to_string(item->getName()),
+                        __FILE__, __LINE__);


There is a nice exception class which captures a context where exception need to report the one:

util::Issue

iagaponenko · 2018-11-08T07:21:11Z

core/modules/loader/CentralMaster.cc

+    // TODO Make a better algorithm, insert workers at busiest worker.
+    // TODO maybe rate limit this check.
+    std::string funcName("_assignNeighborIfNeeded");
+    if (_addingWorker) {


It feels like that check be better done after acquiring a lock on a mutex. Otherwise there is a chance of running into a race condition.

iagaponenko · 2018-11-08T07:22:31Z

core/modules/loader/CentralMaster.cc

+    auto pair = _mWorkerList->getActiveInactiveWorkerLists();
+    std::vector<MWorkerListItem::Ptr>& activeList = pair.first;
+    std::vector<MWorkerListItem::Ptr>& inactiveList = pair.second;
+    if (inactiveList.empty() || _addingWorker) { return; }


The _addingWorker condition was already checked earlier

The check on _addingWorker (which is atomic) earlier is meant to avoid trying to trying to lock _assignMtx, but as you said there is a race condition, so this second check is needed.

Unfortunately, it looks like _addingWorker is is never set back to false, which will take some effort.

iagaponenko · 2018-11-08T07:23:26Z

core/modules/loader/CentralMaster.cc

+    if (inactiveList.empty() || _addingWorker) { return; }
+    double sum = 0.0;
+    int max = 0;
+    uint32_t maxName = 0;


This variable should be named as workerNumber

iagaponenko · 2018-11-08T07:27:41Z

core/modules/loader/CentralMaster.cc

+    }
+}
+
+MWorkerListItem::Ptr CentralMaster::getWorkerNamed(uint32_t name) {


That name is not consistent with its numeric type. Should be workerNumber. And the same applies to the name of the method.

iagaponenko

Reviewed classes CentralWorker and ClientServer

core/modules/loader/CentralClient.cc

iagaponenko · 2018-11-13T02:11:56Z

core/modules/loader/CentralMaster.cc

+    auto pair = _mWorkerList->getActiveInactiveWorkerLists();
+    std::vector<MWorkerListItem::Ptr>& activeList = pair.first;
+    std::vector<MWorkerListItem::Ptr>& inactiveList = pair.second;
+    if (inactiveList.empty() || _addingWorker) { return; }


iagaponenko · 2018-11-13T02:19:55Z

core/modules/loader/CentralWorker.h

+
+// system headers
+#include <boost/bind.hpp>
+#include <boost/asio.hpp>


Boost headers are the "Third-party headers"

iagaponenko · 2018-11-13T02:20:17Z

core/modules/loader/CentralWorker.h

+ *
+ */
+#ifndef LSST_QSERV_LOADER_CENTRAL_WORKER_H_
+#define LSST_QSERV_LOADER_CENTRAL_WORKER_H_


Extra '_' in the end of the macro definition

iagaponenko · 2018-11-13T02:22:09Z

core/modules/loader/CentralWorker.h

+
+namespace proto {
+class WorkerKeysInfo;
+}


Usually we add a comment to the namespace closing parenthesis

} // proto

iagaponenko · 2018-11-14T05:14:51Z

core/modules/loader/ClientServer.cc

+
+
+
+


Too many empty lines in the end of the file

iagaponenko · 2018-11-14T05:15:35Z

core/modules/loader/ClientServer.cc

+    std::unique_ptr<proto::LdrMsgReceived> protoBuf;
+    if (success) {
+        protoBuf = seData->protoParse<proto::LdrMsgReceived>();
+        if (protoBuf == nullptr) { success = false; }


Curly brackets are not needed here.

iagaponenko · 2018-11-14T05:17:25Z

core/modules/loader/ClientServer.h

+ * see <http://www.lsstcorp.org/LegalNotices/>.
+ *
+ */
+#ifndef LSST_QSERV_LOADER_CLIENTSERVER_H_


Extra _ in the end of the macro-definition

iagaponenko · 2018-11-14T05:17:45Z

core/modules/loader/ClientServer.h

+#include <cstdlib>
+#include <iostream>
+#include <boost/bind.hpp>
+#include <boost/asio.hpp>


Boost headers are the third-party headers

iagaponenko · 2018-11-14T05:18:12Z

core/modules/loader/ClientServer.h

+class CentralClient;
+
+
+class ClientServer : public ServerUdpBase {


Please, document the class and its methods!
Please, add deleted:

ClientServer(ClientServer const&) = delete; ClientServer& operator=(ClientServer const&) = delete;

iagaponenko

Reviewed class DoList and other classes in its header

iagaponenko · 2018-11-14T05:23:46Z

core/modules/loader/DoList.cc

+        _list.splice(_list.end(), _addList);
+    }
+    for (auto iter = _list.begin(); iter != _list.end(); ++iter){
+        DoListItem::Ptr const& item = *iter;


I think it's going to be DoListItem::Ptr&

iagaponenko · 2018-11-14T05:24:26Z

core/modules/loader/DoList.cc

+// System headers
+#include <iostream>
+
+// Third-party headers


Please, remove unused comment

iagaponenko · 2018-11-14T05:30:32Z

core/modules/loader/DoList.h

+        if (item == nullptr) return false;
+        if (item->isAlreadyOnList()) return false; // fast atomic test
+        {
+            std::lock_guard<std::mutex> lock(_addListMtx);


The way this method locks the mutex contradicts to the comments to the data members. My understanding is that _listMtx should be always locked first like it's done by method DoList::checkList()

If you have to lock both of them, you must lock _list first. If you know only one will be locked, you only need to lock one of them.

iagaponenko · 2018-11-14T05:31:14Z

core/modules/loader/DoList.h

+ * see <http://www.lsstcorp.org/LegalNotices/>.
+ *
+ */
+#ifndef LSST_QSERV_LOADER_DOLIST_H_


Extra _ in the end of the macro definition

iagaponenko · 2018-11-14T05:33:04Z

core/modules/loader/DoList.h

+    using TimePoint = std::chrono::system_clock::time_point;
+    using Clock = std::chrono::system_clock;
+
+    TimeOut(std::chrono::milliseconds timeOut) : _timeOut(timeOut) {}


Is implicit conversion construction (from std::chrono::milliseconds to TimeOut) expected here?

iagaponenko · 2018-11-14T05:35:32Z

core/modules/loader/DoList.h

+private:
+    /// Lock _mtx before calling.
+    bool _isOneShotDone() {
+        return (!_needInfo && _oneShot);


Parentheses are not needed here

iagaponenko · 2018-11-14T05:37:41Z

core/modules/loader/DoList.h

+    virtual util::CommandTracked::Ptr createCommand()=0;
+
+protected:
+    std::atomic<bool>    _addedToList{false}; ///< True when added to a DoList


I think protected members should NOT begin with _. Here is the link to the relevant section of the Style guide:
https://developer.lsst.io/cpp/style.html#the-parts-of-a-class-must-be-sorted-public-protected-and-private

And this is for private members:
https://developer.lsst.io/cpp/style.html#private-class-variables-and-methods-must-have-underscore-prefix

iagaponenko

Reviewed class LoaderMsg.

iagaponenko · 2018-11-14T20:48:42Z