HPCC-17851 New config manager core library #10773

kenrowland · 2018-01-09T19:02:42Z

Adds the core library for the new configuration manager.

Signed-off-by: Ken Rowland kenneth.rowland@lexisnexisrisk.com

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Testing:

hpcc-jirabot · 2018-01-09T19:02:47Z

https://track.hpccsystems.com/browse/HPCC-17851
Jira not updated (pull request already registered)

kenrowland · 2018-01-09T19:06:24Z

@rpastrana @RussWhitehead @afishbeck

This pull request replaces the previous request. It incorporates the comments from previous reviews. It also includes a refactoring for better naming and class structure. Please review.

Thanks.

rpastrana

@kenrowland several comments. some of them need to be applied in multiple files. there were several files where the diff was too big and github did not provide the diff.
Also, I did not review the new XSDs

rpastrana · 2018-01-19T14:21:07Z

configuration/config2/EnvironmentMgr.cpp

+
+EnvironmentMgr *getEnvironmentMgrInstance(const std::string &envType)
+{
+    EnvironmentMgr *pEnvMgr = NULL;


nullptr instead of NULL

rpastrana · 2018-01-19T14:21:43Z

configuration/config2/EnvironmentMgr.cpp

+EnvironmentMgr *getEnvironmentMgrInstance(const std::string &envType)
+{
+    EnvironmentMgr *pEnvMgr = NULL;
+    if (envType == "XML")


If we're really going to support multiple enttypes we should provide an enumeration

I prefer a string, then if we add a new type, client code does not need to be rebuilt to take advantage of it.

There are many advantages to make this an enumeration. For one, the enumeration itself documents the supported types, no guessing by the developer. An enumerated value leaves no room for casing issues. A test of the enumerated value removes the need for more inefficient string compares. Unless I'm missing something, if we add a new type, the client would need to be rebuilt to request the envType anyway.

rpastrana · 2018-01-19T14:22:48Z

configuration/config2/EnvironmentMgr.cpp

+#include "XMLEnvironmentMgr.hpp"
+
+
+EnvironmentMgr *getEnvironmentMgrInstance(const std::string &envType)


@richardkchapman what's our current policy on using std constructs rather than those provided by jlib?

@rpastrana any chance you discussed this with @richardkchapman offline?

rpastrana · 2018-01-19T14:24:46Z

configuration/config2/EnvironmentMgr.cpp

+}
+
+
+bool EnvironmentMgr::loadSchema(const std::string &configPath, const std::string &masterConfigFile, const std::vector<std::string> &cfgParms)  // todo: add a status object here for return


We frown on letting todo comments reach the upstream branches, let's make a decision to either implement, or abandon the todo.

Todo has been removed.

rpastrana · 2018-01-19T14:25:28Z

configuration/config2/EnvironmentMgr.cpp

+        rc = m_pSchemaParser->parse(configPath, masterConfigFile, cfgParms);
+        if (rc)
+        {
+            m_pSchema->processUniqueAttributeValueSets();  // This must be done first


thumbs up on the comment

rpastrana · 2018-01-19T15:49:24Z

configuration/config2/SchemaValue.cpp

+
+        if (!found)
+        {
+            isValid = false;


we could prob just go ahead and return false here

rpastrana · 2018-01-19T16:02:06Z

configuration/config2/SchemaValue.hpp

+            unsigned m_deprecated: 1;
+            unsigned m_isUnique  : 1;
+            unsigned m_isDefined : 1;
+        } bitMask;


I didn't notice this struct being used in any special way, is there any reason these couldn't be just a series SchemaValue member vars of type bool? and if we wanted to create an actual bit mask, why not use a single unsigned to represent the bitmask (where each of the bits represent a different meaning)?

This is a standard way of defining a bitfield where the members of the struct, bitMask, are single bits (or multiple bits if needed). The reason is to reduce memory usage. Multiple bools each take more memory that compacting all of the bool values into a single bitmask

rpastrana · 2018-01-19T16:05:52Z

configuration/config2/Status.cpp

+#include "Status.hpp"
+
+
+void Status::addStatusMsg(enum statusMsg::msgLevel level, const std::string &nodeId, const std::string &name, const std::string &referNodeId, const std::string &msg)


This status class has a generic name, but it seems to be specific to validate() actions (at least this method makes it seem specific to actions involving nodeIds).
If that's the case let's label it as such "ValidateStatus"
otherwise, let's provide a truly generic addStatusMsg (enum statusMsg::msgLevel level, const std::string &msg) as well.

I changed the method per our conversation and provided overloads that eliminate the unneeded parameters.

rpastrana · 2018-01-19T16:10:56Z

configuration/config2/EnvironmentMgr.cpp

+    {
+        m_message = "Unable to open environment file '" + filename + "'";
+    }
+    return rc;


Ensure the stream is closed if it needs to be closed.

Destructor for ifstream will close it on exit from method.

rpastrana · 2018-01-19T16:15:18Z

configuration/config2/configfiles/buildset.xml

+<?xml version="1.0" encoding="utf-8"?>
+<!--
+################################################################################
+#    HPCC SYSTEMS software Copyright (C) 2012 HPCC Systems®.


the 2012 copyright date indicates this file has been around since then, but it appears to be new (at least in this location). Are we moving the original buildset, or is this a placeholder, or are we going to have multiple buildset.xmls?

This is still the original. I haven't made any changes

We will have at least two buildsets for the time being. I am still looking at the long term need to keep buildset. for now I changed the date.

ok, I'm actually not sure if the date should remain at 2012 since it's a pre-existing file or not. We're prob better off keeping the established date. of more concern is the fact that we have duplicate files now. Why can't we just use the original buildset only?

based on offline conversation with @kenrowland this file is an exact copy of original buildset.xml and is here as placeholder. I'm not crazy about this approach, but for the sake of progress @kenrowland will open a jira to address this in a separate jira.

Jira https://track.hpccsystems.com/browse/HPCC-19032 created to ensure this is removed.

afishbeck · 2018-01-26T21:22:03Z

.gitignore

@@ -6,3 +6,4 @@ ln/
 node_modules
 esp/src/lib
 esp/src/build
+.vscode/


What does this do?

VS code creates a .vscode directory where you open a project. In three is stores settings for the component such as debug process information. I felt it best to simply add it to the ignore list to ensure any of these files were never committed accidentally.

afishbeck · 2018-01-26T22:12:06Z

configuration/config2/configfiles/buildset.xml

+      </BuildSet>
+    </Build>
+  </Programs>
+</Environment>


add newline?

afishbeck · 2018-01-26T22:13:34Z

configuration/config2/configfiles/environment.xml

@@ -0,0 +1,1143 @@
+<?xml version="1.0" encoding="UTF-8"?>


How is this instance file used?

This is a test file for now and will be removed in a forthcoming PR

afishbeck · 2018-01-26T22:14:49Z

configuration/config2/configfiles/types.xsd

+        <xs:attribute name="build" type="xs:string" use="required" hpcc:hidden="true" hpcc:tooltip="The build name to be deployed"/>
+        <xs:attribute name="buildSet" type="xs:string" use="required" hpcc:hidden="true" hpcc:autoGenerateType="configProperty" hpcc:autoGenerateValue="componentName"/>
+    </xs:attributeGroup>
+</xs:schema>


Github wants returns at the end of these files.

afishbeck · 2018-01-26T22:18:55Z

configuration/config2/test.cpp

+        // 158
+        //auto pNode = envMgr.getNodeFromPath("158");
+        // auto pNode = pEnvMgr->getEnvironmentNode("74");     // 29 is Hardware/Computer
+        auto pNode = pEnvMgr->getEnvironmentNode("35");


How are these Ids generated? Hardware/Computer will always be 29?

IDs are generated during parsing of the environment. Each EnvironmentNode is given an ID value when added to the internal environment tree. IDs are immutable until a new environment is loaded. IDs are also be be treated as opaque values and not interpreted, only used to address a node. If a node is deleted, that ID is no longer valid and will not be recycled during a session with an environment file. Newly added nodes are assigned new IDs.

IDs in test.cpp were previously determined and hardcoded for testing purposes only.

afishbeck · 2018-01-26T23:09:54Z

configuration/config2/EnvironmentNode.cpp

+}
+
+
+std::vector<std::shared_ptr<SchemaItem>> EnvironmentNode::getInsertableItems() const


Don't quite understand what "insertable" means.

Insertable items is a collection of components or other environment items that can be added to the current node. For example, an Instance for an additional node for an ESP service. It could also be an ESP service as well. The method takes into account the number of allowed instances specified in the XSD file.

afishbeck · 2018-01-26T23:14:07Z

configuration/config2/EnvironmentMgr.hpp

+        std::string getLastSchemaMessage() const;
+        std::string getLastEnvironmentMessage() const { return m_message;  }
+        bool loadEnvironment(const std::string &qualifiedFilename);
+        std::shared_ptr<EnvironmentNode> getEnvironmentNode(const std::string &nodeId);


Maybe you should walk me through how these IDs work.

afishbeck · 2018-01-26T23:17:03Z

configuration/config2/EnvironmentNode.hpp

+        std::shared_ptr<SchemaItem> m_pSchemaItem;
+        std::weak_ptr<EnvironmentNode> m_pParent;
+        std::multimap<std::string, std::shared_ptr<EnvironmentNode>> m_children;
+        std::shared_ptr<EnvironmentValue> m_pNodeValue;   // the node's value (not normal)


Should this inherit an EnvironementValue, rather than contain one?

The EnvironmentValue is set during parsing if one is found in the environment. Since this is not common, it may not have one.

afishbeck · 2018-01-26T23:20:20Z

configuration/config2/EnvironmentValue.hpp

+        bool wasForced() const { return m_forcedSet; }
+        bool isValueValid(const std::string &value) const;
+        void validate(Status &status, const std::string &myId) const;
+        std::vector<std::string> getAllValues() const;


This is a little strange. "Get the value of me and all my siblings"?

I know it's a little strange, but it's used for key and keyref processing. the method is a bit of a convenience method since it uses it's name (attribute name) and calls it's parent which actually does the search of all siblings. Without this convenience method, the caller would have to do several calls. Perhaps getAllValuesLikeMe or getAllSiblingValues?

afishbeck · 2018-01-26T23:28:41Z

configuration/config2/ValueDef.hpp

+#include <string>
+#include "platform.h"
+
+struct ValueDef {


Would "AttributeDef" or "NameValueDef" sometimes be more clear?

Changed it to NameValue

kenrowland · 2018-01-30T18:31:29Z

@rpastrana @afishbeck Review comments incorporated or addressed with comments. Please let me know if there are any additional comments.

rpastrana

@kenrowland approved. please create the jira for removing the second buildset.xml

afishbeck

@kenrowland looks fine. Further review will come when the web service is ready.

kenrowland · 2018-01-31T20:05:30Z

@richardkchapman Please merge. I tried to squash, but had some merge problems. If you need me to, I will abandon this PR and create a new one with a new single commit branch.

richardkchapman · 2018-02-01T07:54:12Z

@kenrowland Please do squash via whatever means are necessary.

Adds the core library for the new configuration manager. Signed-off-by: Ken Rowland <kenneth.rowland@lexisnexisrisk.com>

HPCCSmoketest · 2018-02-01T21:40:16Z

Automated Smoketest: ✅
Sha: 0550ad5
Build: success
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test	total	passed
unittest	81	81
wutoolTest(Dali)	19	19
wutoolTest(Cassandra)	19	19

Regression test result:

phase	total	pass
setup (hthor)	11	11
setup (thor)	11	11
setup (roxie)	11	11
test (hthor)	741	741
test (thor)	650	650
test (roxie)	768	768

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time	Build time	Package time	Install time	Start time	Test time	Stop time	Summary
15 sec (00:00:15)	182 sec (00:03:02)	55 sec (00:00:55)	6 sec (00:00:06)	37 sec (00:00:37)	1106 sec (00:18:26)	33 sec (00:00:33)	1434 sec (00:23:54)

kenrowland mentioned this pull request Jan 9, 2018

HPCC-17851 Phase 1 new configuration manager - core library #10491

Closed

36 tasks

rpastrana requested changes Jan 19, 2018

View reviewed changes

afishbeck reviewed Jan 26, 2018

View reviewed changes

rpastrana approved these changes Jan 30, 2018

View reviewed changes

afishbeck approved these changes Jan 31, 2018

View reviewed changes

kenrowland force-pushed the hpcc-17851 branch from 165947c to 117c793 Compare February 1, 2018 20:52

HPCC-17851 New config manager core library

0550ad5

Adds the core library for the new configuration manager. Signed-off-by: Ken Rowland <kenneth.rowland@lexisnexisrisk.com>

kenrowland force-pushed the hpcc-17851 branch from 117c793 to 0550ad5 Compare February 1, 2018 21:00

richardkchapman merged commit 7e9390d into hpcc-systems:master Feb 2, 2018

		#include "XMLEnvironmentMgr.hpp"


		EnvironmentMgr *getEnvironmentMgrInstance(const std::string &envType)

		}


		bool EnvironmentMgr::loadSchema(const std::string &configPath, const std::string &masterConfigFile, const std::vector<std::string> &cfgParms) // todo: add a status object here for return

		#include "Status.hpp"


		void Status::addStatusMsg(enum statusMsg::msgLevel level, const std::string &nodeId, const std::string &name, const std::string &referNodeId, const std::string &msg)

		}


		std::vector<std::shared_ptr<SchemaItem>> EnvironmentNode::getInsertableItems() const

HPCC-17851 New config manager core library #10773

HPCC-17851 New config manager core library #10773

Conversation

kenrowland commented Jan 9, 2018 • edited

Type of change:

Checklist:

Testing:

hpcc-jirabot commented Jan 9, 2018

kenrowland commented Jan 9, 2018

rpastrana 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

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

kenrowland Jan 22, 2018 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenrowland Jan 29, 2018 • 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

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

Choose a reason for hiding this comment

kenrowland commented Jan 30, 2018

rpastrana left a comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

kenrowland commented Jan 31, 2018

richardkchapman commented Feb 1, 2018

HPCCSmoketest commented Feb 1, 2018

kenrowland commented Jan 9, 2018 •

edited

kenrowland Jan 22, 2018 •

edited

kenrowland Jan 29, 2018 •

edited