Tickets/dm 17656 #508

jgates108 · 2019-12-13T18:19:46Z

No description provided.

…ents.

Code cleaned up, system tested with 100k inserts and lookups.

… test.

…/DM-17656

iagaponenko

I have made the first sweep through classes "appClientNum, appClientMaster, appWorker, BufferUdp, CentralFollower, CentralClient, CentralWorker, ClientServer, WorkerServer, WWorkerList" and files in folder "admin/tools/docker/index". I have left comments where I could understand the code. My main problem was that this is not the usual "diff" ticket. It appears as a completely new development. So, it makes it difficult to judge how the claimed goal of the ticket has been achieved with this code.
My second big complain with the code is the naming convention behind many classes and some methods. The naming convention is way to abstract/generic. It makes it difficult to understand roles and behaviors of classes and methods.
Ideally I would like to have an in-person conversation to better understand the overall design of this application.
I approve it, conditionally, provided that my comments and suggestions will be considered.

iagaponenko · 2019-12-19T21:56:32Z

admin/tools/docker/index/container/buildContainers.bash

@@ -0,0 +1,31 @@
+#! /bin/bash


Based on my understanding on what the script does it's supposed to be launched from (cd <path>) some specific directory (which is the same one where the script runs. This opens numerous possibilities for making mistakes. I would recommend passing a target folder as a parameter to the script, so that it could be run like:

% qserv/admin/tools/docker/loader/container/buildContainers.bash <qserv-base-dir>

You may get and evaluate a value of the parameter with:

QSERV_BASE_DIR="$1" if [ -z "$QSERV_BASE_DIR" ]; then echo "usage: <path>" exit 1 fi cd $QSERV_BASE_DIR

iagaponenko · 2019-12-19T22:12:42Z

admin/tools/docker/index/container/dev/clientNum/Dockerfile

@@ -0,0 +1,13 @@
+#


What's a point in building a separate binary container for each tool (clientNum, appClientNum, appClientNumScreen, etc.)? You may as well package all these binaries into a single container. Then you may launch the container by specifying an application you want to run:

% docker run -it --rm qserv/index:dev clientNum.bash <parameters>

I think you may just add the name of a script/binary as an extra argument in the Kubernetes YAML file:

args: ["clientNum.bash", "10000000", "1", "client-k8s-a1.cnf"]

It made the yaml scripts easier to write and get working (which was not easy) and an actual instance of the system would not have the client apps, as they are only for testing. It would make sense to eventually trim down the containers so they only contain what they really need.
Currently, the containers all share the same qserv/indexbase:dev. The differences for the individual containers really comes down the entrypoint designations and bash scripts, which is small. There could be significant differences between master and worker containers in the future.

iagaponenko · 2019-12-19T22:15:11Z

admin/tools/docker/index/container/dev/clientNum/appClientNumScreen.bash

+screen -dm /home/qserv/dev/qserv/admin/tools/docker/index/container/dev/clientNum/appClientNum $1 $2 $3 
+
+
+tail -f /dev/null


The NEWLINE symbol is missing in the end of the file

iagaponenko · 2019-12-19T22:18:14Z

admin/tools/docker/index/container/buildContainers.bash

@@ -0,0 +1,31 @@
+#! /bin/bash


I would also recommend adding:

set -e

to this and all other shell scripts. This will terminate the on errors and prevent unreliable results. See more on tis at:
https://stackoverflow.com/questions/6930295/set-e-and-short-tests

iagaponenko · 2019-12-19T22:29:34Z

admin/tools/docker/index/container/dev/worker/appWorkerScreen.bash

+
+screen -dm /home/qserv/dev/qserv/admin/tools/docker/index/container/dev/worker/appWorker.bash
+
+tail -f /dev/null


The NEWLINE symbol is missing in the end of the file

iagaponenko · 2019-12-20T03:00:22Z

core/modules/loader/CentralWorker.h

+
+    int getTcpPort() const override { return _tcpPort; }
+
+    uint32_t getOurId() const {


Why this method is defined as inline?

iagaponenko · 2019-12-20T03:02:07Z

core/modules/loader/CentralWorker.cc

@@ -0,0 +1,1146 @@
+// -*- LSST-C++ -*-


This file is huge. I happen to review it many months ago. And it's not clear from this PR what were exact changes to the class in a context of this development.

iagaponenko · 2019-12-20T03:02:37Z

core/modules/loader/CentralWorker.cc

+#include "loader/CentralWorker.h"
+
+// system headers
+#include <boost/asio.hpp>


The third party header. And it must be included in the double quotes.

iagaponenko · 2019-12-20T03:04:15Z

core/modules/loader/ClientServer.h

+#include <iostream>
+
+// third party headers
+#include <boost/asio.hpp>


The non-system headers should not be included with the angle brackets.

iagaponenko · 2019-12-20T03:07:10Z

core/modules/loader/WorkerServer.h

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


These two are the third party headers, and they also need to be included within the doube quotes.

jgates108 added 30 commits October 24, 2019 12:56

Modified FileServer and added lader to build.

24fdf1c

Modified FileServerConnection.

8657cc9

Modified FileServer and added lader to build.

e3723a2

Modified FileServerConnection.

757bdf6

Added udp test code.

64d7678

Test binaries.

1747b79

Changed to echo udp server and client.

c650198

Some progress.

17875a5

Added SConscript file.

b5c56c2

Working UDP async echo server and sync client.

f13a1c6

Implemented basic master and worker.

6fe9cf8

Fixed errors from rebase.

263e92f

Changes to handleTest2

6d5e162

TCP server working test.

9b170d2

Passing basic tests, again.

7d8db62

Added code so worker recognizes new neighbor.

7d012c9

Added files.

9307c28

Added range setting logic.

bf2bce5

Range setting changes.

e147bcd

Shift to right almost working.

7cec788

Shift to right appears to work.

ab197f9

Fixed some bugs. Added ability to send a key insert to a neighbor.

2613224

Shifting appears to work.

e22c5d0

Fixed communication problem.

b000de0

Added code so the master will send ranges to all its clients.

fc9d1c7

Cleaned up code.

02fba9e

More code cleanup.

51739ae

More code cleanup.

b0be4f6

Removed unused files.

e2891ea

Made some review changes.

868aace

jgates108 added 24 commits October 29, 2019 16:05

Removed a comment.

c95a543

Add configuration file classes.

0a40045

Fixed a bug with TCP communications and added configuration file elem…

e7c2513

…ents.

Cleaned up code.

9a95c3e

Added reasonable type checking to configuration file values.

15c7689

Implemented max number of concurrent client insert and lookup requests.

2358c35

Started converting to using CompositeKey instead of string keys.

364d629

Changed to use CompositeKey instead of string for keys.

ac60356

Created separate programs for master, client, and server.

36da2ce

Code cleaned up, system tested with 100k inserts and lookups.

Added timers to client program.

4752dc7

Added files for configuring kubernetes.

865b44c

Kubernetes configuration added.

2ed287e

Added configuration for appClientNum.

0b100c5

Code now inserts keys into the system.

a35dda8

Added multiple clients to kuebernetes configuration.

16a53d2

Significantly reduced the number of concurrent insert attempts in k8s…

dd92d7f

… test.

Increased the size of the buffer used for shifts.

8e1e1de

Added comments.

2138a67

Made changes to indexer Dockerfile.

d6f9a49

Fixed Dockerfile.

4defa29

Merge branch 'tickets/DM-17656' of github.com:lsst/qserv into tickets…

b1b0d2d

…/DM-17656

MAde changes to container build files.

ccd0bcc

Made changes so kubernetes can terminate containers.

a01379b

Fixed race condition in MsgElement::retrieve.

914341a

jgates108 force-pushed the tickets/DM-17656 branch from fb83e32 to ec0d6d7 Compare December 13, 2019 18:31

Changed to keep map of DNS addresses due to 5 second DNS delays.

58f783e

jgates108 force-pushed the tickets/DM-17656 branch from ec0d6d7 to 58f783e Compare December 13, 2019 22:50

iagaponenko approved these changes Dec 20, 2019

View reviewed changes

Changed to lazy initialization for CentralFollower::_wWorkerList.

ee9ebde

jgates108 force-pushed the u/jgates/loader branch from 6f944aa to 6f0ba51 Compare January 22, 2021 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tickets/dm 17656 #508

Tickets/dm 17656 #508

jgates108 commented Dec 13, 2019

iagaponenko left a comment

iagaponenko Dec 19, 2019

iagaponenko Dec 19, 2019

jgates108 Jan 6, 2020

iagaponenko Dec 19, 2019

iagaponenko Dec 19, 2019

iagaponenko Dec 19, 2019

iagaponenko Dec 20, 2019

iagaponenko Dec 20, 2019 •

edited

iagaponenko Dec 20, 2019

iagaponenko Dec 20, 2019

iagaponenko Dec 20, 2019

		screen -dm /home/qserv/dev/qserv/admin/tools/docker/index/container/dev/clientNum/appClientNum $1 $2 $3


		tail -f /dev/null


		int getTcpPort() const override { return _tcpPort; }

		uint32_t getOurId() const {

Tickets/dm 17656 #508

Are you sure you want to change the base?

Tickets/dm 17656 #508

Conversation

jgates108 commented Dec 13, 2019

iagaponenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iagaponenko Dec 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iagaponenko Dec 20, 2019 •

edited