Nameserver #103

AlexRast · 2019-09-25T23:40:41Z

This adds the NameServer process and makes the Mothership inherit from SBase. Now that Mothership officially inherits from its expected base classes, its class files are given the 'proper' Mothership.h and Mothership.cpp names. I think Ns_el.h and Ns_el.cpp are probably not needed but I have not changed them and so far everything seems to work - tested with some small working examples using both default and application supervisors from heat plate and clock tree, and with some larger (multi-board) examples as well.
There is a MothershipDummy class to aid in testing of the NameServer without the full mothership; it can probably eventually be dropped but might still come in handy when testing UserIO in future.
I have added a few output messages when loading cores just to give the operator some sense of progress - note how SLOW these are to remind everyone of how valuable a thread safe multi-core/multi-board loader would be!
I have manually merged non-conflicting updates to other files from the development branch so although it may show as being any number of commits behind (reflecting the point when the branch was created!) in fact most of the actual files should be up-to-date; of course with significant changes to nameserver, mothership and several related files including the PMsg_p and CMsg_p classes.
For now the group of name services that can inject packets into a Tinsel core directly based upon a device name is not implemented, pending implementation of UserIO. But most of the rest of the name services commands are implemented and tested.

mvousden · 2019-09-26T08:23:37Z

Can we merge development into nameserver, so that the review only displays the changes you've made (as opposed all the changes that have been made since ca8bbaf)? i.e.:

git pull
git checkout nameserver
git merge development
... # resolve merge conflicts, potentially with some help
... # resolve changes introduced from renaming TMoth.{h,cpp} -> Mothership.{h,cpp}, potentially with some help
git commit
git push

If you're not confortable to do this, we can do this together, or explore other avenues. As it stands, it's difficult for me (as a reviewer) to see only the changes that you have introduced.

I'm around this afternoon (from about 1530).

…he last commit

…er commit fixing bad reversions, etc. immediately after this

heliosfa · 2019-09-26T14:10:31Z

Just to echo what Mark said, this need to have Development merged in properly (rather than manually) before it can be merged back into development.

It is incredibly bad practice to review and/or merge when there are conflicting files - the merge should be a clean one.

AlexRast · 2019-09-26T15:10:41Z

As seen (3 commits later...) this should fix everything.

mvousden

Thanks for opening the PR. I appreciate the comments and line-splitting you've added to Mothership, and the progress you've introduced towards external device support.

Points:

Issues labelled with sweet potatoes (:sweet_potato:) are issues that I would not be comfortable with keeping in the source - to get my approval, address the issue, or convince me that it's not a problem.
I remember that you had some documentation for the Mothership floating about; has it been updated with these changes? Either way, could you point me to it?
As a general rule, please keep code repetition to a minimum. There's a lot of copy-paste.
You mention you've tested these changes. Can you elaborate on how exactly you've tested it? Have you tried running the Nameserver on a different box? Presumably you have some check that involves loading an application and dumping the state - if so, can you share your script?
You say "Tested with some small working examples using both default and application supervisors from heat plate and clock tree, and with some larger (multi-board) examples as well". This is good - @heliosfa and I test our changes with some iteration of the heated plate example. Could you upload the XMLs you've tested with? If too large, could you dropoff/safesend it to @heliosfa and myself?
You say "I think Ns_el.h and Ns_el.cpp are probably not needed but I have not changed them and so far everything seems to work". Can you remove these? It's important not to have unused source files in the tree, because it only leads to confusion about what is in use, and what isn't.
When I compile these changes, I get some warnings (
warnings.txt). Please address these (they all look largely trivial). 🍠

Again, thanks for your work, looking forward to your response.

mvousden · 2019-09-27T08:24:28Z

Build/gcc/Makefile


+# Path to the root directory of the Orchestrator (not to be confused with the
+# root process of the Orchestrator).
+ROOT_DIR = ../..


This is a duplicate definition - it's already defined in Makefile.dependencies included in L23. 🍠

Build/gcc/Makefile

mvousden · 2019-09-27T08:28:09Z

Build/gcc/Makefile.executable_prerequisites

+                        $(SOURCE_DIR)/NameServer/ABRecord.cpp \
+                        $(SOURCE_DIR)/NameServer/ABTask.cpp \
+                        $(SOURCE_DIR)/NameServer/AddressBook.cpp \
+		        $(SOURCE_DIR)/NameServer/SBase.cpp \


Indentation miss here, and in a couple of other places in this file (the lines are justified using spaces, not tabulator characters; we should be consistent) 🍠

I hate tabs. I hate the person who thought of the concept of a tab character. Whoever it was (probably someone at IBM), they should have been forced to use EBCDIC coding for the rest of their life. I also hate editors that automatically insert tabs. Did I say I hate tabs?...

Add:

(setq-default indent-tabs-mode nil) | (setq-default tab-width 4)

to your .emacs if don't want the tab key to insert tabs in emacs.

mvousden · 2019-09-27T08:29:23Z

Build/gcc/Makefile.executable_prerequisites

+		        $(SOURCE_DIR)/OrchBase/P_addr.cpp 
+
+# The LogServer message file is a static text file
+LOGSERVER_MESSAGE_FILE_ORIGIN = $(SOURCE_DIR)/OrchestratorMessages.txt


Duplicate definition with L196. 🍠

This looks like something inserted during the merge. DOes Git do this (interleave files?) From reading the documentation it looked ats if it might, which is why I was very VERY reluctant to do a merge, but it looks also as though it does it so subtly that you can't even do a compare to detect differences. If merges are required, on the one hand, and if merge silently interleaves files on the other, and if it doesn't give you an option that allows you to do a line-by-line approval of the merge as it happens, I don't see how it can be possible to pull a branch back into the main line without creating merge issues.

This additional line was introduced by commit 2bbe0a6, which was before development was merged in:

commit 2bbe0a6f80930a234fa5d7da04153f833dd9cddd Author: AlexRast <adr1r17@soton.ac.uk> Date: Thu Sep 26 14:48:22 2019 +0100 re-updated makefiles and message file that were somehow reverted in the last commit

Perhaps your issue is with the work prior to this commit.

Build/gcc/Makefile.executable_prerequisites

mvousden · 2019-09-27T10:29:40Z

Source/NameServer/AddressBook.cpp

+//==============================================================================
+namespace AddressBookNS
+{
+#ifdef EXCEPTS


I've not seen this pattern before! Seems a little odd because you'll need to define X and Y in your source, even if they aren't needed...

This was mine when I was considering having throwable exceptions.
I meant to talk to you about your preference - The exceptions really did not work well with Borland (it insisted on stepping down the exception tree) hence the return code option.

I prefer exceptions for what you're doing here, because chains of break statements can be considerably more difficult to follow, but I'm pretty ambivalent.

I'd rather you choose one or the other for "production" - otherwise this becomes a bit of a Frankenstein amalgam.

mvousden · 2019-09-27T10:29:52Z

Source/NameServer/AddressBook.cpp

+#define ERETURN(X, Y) return Y
+#endif
+
+// static constant definitions


Use an enum.

I think this has to do with Graeme's comment above as well.

Mark's comment was in regard to the following list of consts.

@heliosfa has it right here.

mvousden · 2019-09-27T10:31:11Z

Source/NameServer/AddressBook.cpp

+//Task Manipulation
+unsigned AddressBook::AddTask(const std::string &TaskName, TaskData_t &Data)
+{
+    // Check that the task does not already exist.


FWIW, this kind of repetition is fine because the actual check is abstracted into a method - the duplication is as small as it reasonably can be.

mvousden · 2019-09-27T10:33:16Z

Source/NameServer/AddressBook.hpp

+
+private:
+    TaskMap_t TaskMap;
+    int TaskCount;


Why do you need to maintain/update this variable? Surely you can just do TaskMap_t.size().

Graeme? Does this have an important use? Are you just trying to minimise the calls to the size() function?

I think I was trying to minimise the calls to size() - it is also a good metric to looks for "oopsies" to the data structure.

To me, TaskMap holds one record for each task. The number of tasks is small, so surely the code complexity added by "caching" a size() call is not worth just calling size() on this very small map.

I think you'll introduce more "oopsies" than you'll catch with this extra maintenance :p

Source/NameServer/AddressBook.cpp

heliosfa · 2019-09-27T11:08:07Z

When I compile these changes, I get some warnings (
warnings.txt). Please address these (they all look largely trivial). 🍠

A lot of these warnings look like a reversion of the changes made in #88 to address #31 to suppress warnings (e.g. d9340cc, 1faa0a9 and others look to have been undone)

AlexRast · 2019-09-30T22:38:17Z

@heliosfa's last comment doesn't seem to have a reply option - but is this something that could have happened because of the merge process? I would not be surprised. I don't remember seeing warnings pre-merge.

AlexRast · 2019-09-30T23:30:14Z

Some of the warnings look to have been related to merging, others though when I looked through them would clearly have been there anyway. In any case, these are all fixed together with removal of tabs and the makefile stuff.

AlexRast · 2019-09-30T23:34:16Z

Can I get everyone's acknowledgement that Ns_el.h/Ns_el.cpp are not in use nor intended for some future use? I have kept them in just in case someone screamed 'NOOO, don't delete that, we need it for xxx/I was planning to use it for yyy'.

heliosfa · 2019-09-30T23:35:33Z

Can I get everyone's acknowledgement that Ns_el.h/Ns_el.cpp are not in use nor intended for some future use? I have kept them in just in case someone screamed 'NOOO, don't delete that, we need it for xxx/I was planning to use it for yyy'.

I believe that you are correct - they are ADB's original pre-implementation

heliosfa · 2019-09-30T23:37:35Z

@heliosfa's last comment doesn't seem to have a reply option - but is this something that could have happened because of the merge process? I would not be surprised. I don't remember seeing warnings pre-merge.

Depending how the merge was done, potentially.

One of the changes you have merged in is adding -Wall and -Wpedanticto the make file, which turn on more warnings. The head of development builds cleanly with these flags, so any warning have been introduced by your changes/the merge you have done.

… tabs replaced with spaces

mvousden · 2019-10-01T09:21:02Z

Can I get everyone's acknowledgement that Ns_el.h/Ns_el.cpp are not in use nor intended for some future use? I have kept them in just in case someone screamed 'NOOO, don't delete that, we need it for xxx/I was planning to use it for yyy'.

Agree, get rid of it (we can always pull it out of the history if it becomes relevant again for some presently unknown reason).

AlexRast · 2019-10-01T12:53:20Z

Yes, I think the -Wpedantic in particular was responsible for a lot of the additional warnings coming up.

… NameServer searching

…ce, a few small cosmetic changes

mvousden

A set of responses and resolutions.

Source/Common/CommonBase.h

mvousden · 2019-10-08T08:59:40Z

Source/Common/SDecode.cpp

@@ -0,0 +1,52 @@
+


The issue here is with the design of "Decode". One of the core problems is, because it posts, you can't define a stack of decode methods that get called in sequence (this is why logging is tricky to get right - methods like Post result in inherently brittle, nonmodular code, but other solutions are painful too).

I think you're right, let's accept the duplication. Can you please make a note in the comment at the head of the file explaining that this is a near copy-paste of decode (and vice-versa)?

mvousden · 2019-10-08T09:06:01Z

Source/NameServer/AddressBook.cpp

+//==============================================================================
+namespace AddressBookNS
+{
+#ifdef EXCEPTS


I prefer exceptions for what you're doing here, because chains of break statements can be considerably more difficult to follow, but I'm pretty ambivalent.

I'd rather you choose one or the other for "production" - otherwise this becomes a bit of a Frankenstein amalgam.

mvousden · 2019-10-08T09:07:28Z

Source/NameServer/NameServer.h


+        unsigned    Connect          (string="");
+        unsigned    OnCfg            (PMsg_p *, unsigned);
+	unsigned    OnDump           (PMsg_p *, unsigned);


Indentation

mvousden · 2019-10-08T09:18:00Z

Source/NameServer/SBase.cpp

+//==============================================================================
+unsigned SBase::QueryDevIAll(PMsg_p *msg, unsigned comm)
+{
+   string taskName = msg->Zname(0); // as usual, first static field is the task


The whole thing is structured so that many of the fields in the message contain the same information retrieved in the same way.

So why not abstract it? By not abstracting it, you're brittle to any potential change in the structure later.

But I am extracting them in each command-response directly so that when looking through the code you don't have to go hunting through multiple functions for each little bit of extraction

If you read these query methods in sequence, and if you call the same method each time, it's more clear to a reader that you are doing the same thing. The reader looks at the function once (at most) and, if the method is named appropriately, you won't need to look at the definition to understand what it does.

hunting through multiple functions for each little bit of extraction

It's trivial to lookup a method in this way - the call stack here is pretty flat, and the function would be in the same file.

We're still several levels away from "over-atomisation" in my opinion. I argue that since you reduce code size, duplication, and improve clarity and modularity at the cost of one stack level, the tradeoff of abstraction is certainly worth it here.

"In my opinion" being the key clause here.

…ive TMoth.cpp from the dead even though I want to kill it permanently

…grabs an entire flit

…biguation of a Union

…103

AlexRast and others added 6 commits July 19, 2019 14:31

Added nameserver process

f8502d4

removed debug file and added a few fixes for state update

de6273f

added distribution functions and a dummy Mothership for testing

5336f1c

added support for queries and responses in SBase and dummy Mothership

7c61b0d

Commenting and some small fixes

0d3c803

Mothership with full SBase added and some debug-output cleanup

db6112b

AlexRast requested review from heliosfa and mvousden September 25, 2019 23:40

Some extra messaging output during loading

76f0fd4

mvousden assigned AlexRast Sep 26, 2019

AlexRast added 2 commits September 26, 2019 14:48

re-updated makefiles and message file that were somehow reverted in t…

2bbe0a6

…he last commit

This is the merged result of nameserver and development. Expect anoth…

b6ea460

…er commit fixing bad reversions, etc. immediately after this

OK, this probably fixes merge mismatches

c90afb9

mvousden requested changes Sep 27, 2019

View reviewed changes

Fixing merge issues: warnings removed, duplicate makefile lines gone,…

2eba942

… tabs replaced with spaces

AlexRast added 2 commits October 1, 2019 16:50

corrected most of MLV's crucial comments except for the one regarding…

cd81969

… NameServer searching

Ns_el removed, link /nser command temporarily disabled as a consequen…

008048d

…ce, a few small cosmetic changes

mvousden self-requested a review October 8, 2019 08:46

mvousden reviewed Oct 8, 2019

View reviewed changes

AlexRast added 3 commits October 15, 2019 16:45

Comments in Decode.cpp and bugfix for issue 93 incorporated

cfdcd57

I hope this fixes a conflict! Git merge seems to be determined to rev…

bf5fdb2

…ive TMoth.cpp from the dead even though I want to kill it permanently

updated Mothership to match Matt's new HostLink receive which always …

01d63dd

…grabs an entire flit

heliosfa added a commit that referenced this pull request Jan 13, 2020

Addressing @mvousden's comment in #103 to "Use an enum."

5694fbb

heliosfa added a commit that referenced this pull request Jan 13, 2020

Addressing @mvousden's comment in #103 regarding the ERETURN pattern

cf481a7

heliosfa added a commit that referenced this pull request Jan 13, 2020

Adding comment to address @mvousden's comment in #103 regarding disam…

b3001bd

…biguation of a Union

heliosfa added a commit that referenced this pull request Jan 13, 2020

Removing superfluous TaskCount variable as per @mvousden's comment in #…

b610401

…103

heliosfa mentioned this pull request Jan 13, 2020

Addressbook #121

Merged

Nameserver #103

Are you sure you want to change the base?

Nameserver #103

Uh oh!

Conversation

AlexRast commented Sep 25, 2019

Uh oh!

mvousden commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heliosfa commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexRast commented Sep 26, 2019

Uh oh!

mvousden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvousden Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexRast Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

heliosfa commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexRast commented Sep 30, 2019

Uh oh!

AlexRast commented Sep 30, 2019

Uh oh!

AlexRast commented Sep 30, 2019

Uh oh!

heliosfa commented Sep 30, 2019

Uh oh!

heliosfa commented Sep 30, 2019

Uh oh!

mvousden commented Oct 1, 2019

Uh oh!

AlexRast commented Oct 1, 2019

Uh oh!

mvousden commented Sep 26, 2019 •

edited

Loading

heliosfa commented Sep 26, 2019 •

edited

Loading

mvousden Oct 1, 2019 •

edited

Loading

AlexRast Sep 30, 2019 •

edited

Loading

heliosfa commented Sep 27, 2019 •

edited

Loading