HPCC-2964 Configmgr - Allow swapping of Thor Master #3517

Merged
merged 2 commits into from Oct 17, 2012

Conversation

Projects
None yet
4 participants
Member

garonsky commented Oct 15, 2012

  • For Thor Cluster - Topology, add context menu option 'Swap Master' to allow
    selection of a different Thor master.

Signed-off-by: Gleb Aronsky gleb.aronsky@lexisnexis.com

Gleb Aronsky HPCC-2964 Configmgr - Allow swapping of Thor Master
- For Thor Cluster - Topology, add context menu option 'Swap Master' to allow
  selection of a different Thor master.

Signed-off-by: Gleb Aronsky <gleb.aronsky@lexisnexis.com>
799162d

Jira updated

Owner

richardkchapman commented Oct 16, 2012

@jakesmith Please review

@jakesmith jakesmith commented on an outdated diff Oct 16, 2012

deployment/deployutils/configenvhelper.cpp
sName.appendf("temp%d", i + 1);
// Add process node
IPropertyTree* pProcessNode = createPTree(szType);
- pProcessNode->addProp(XML_ATTR_NAME, sName);
- pProcessNode->addProp(XML_ATTR_COMPUTER, computers[i]->queryProp(XML_ATTR_NAME));
- if (nPort != 0) pProcessNode->addPropInt(XML_ATTR_PORT, nPort);
+
+ if(szType == NULL || strcmp(szType, XML_TAG_THORMASTERPROCESS) != 0 || !pThor->queryPropTree(sThorMasterProcess.str()))
+ {
+ pProcessNode->addProp(XML_ATTR_NAME, sName);
+ pProcessNode->addProp(XML_ATTR_COMPUTER, computers[i]->queryProp(XML_ATTR_NAME));
+ if (nPort != 0) pProcessNode->addPropInt(XML_ATTR_PORT, nPort);
addNode(pProcessNode, pThor);
@jakesmith

jakesmith Oct 16, 2012

Member

pProcessNode leaks if not added

@jakesmith jakesmith commented on an outdated diff Oct 16, 2012

deployment/deployutils/configenvhelper.cpp
addNode(pProcessNode, pThor);
- }
+ }
+ else
+ {
+ pThor->queryPropTree(sThorMasterProcess.str())->setProp(XML_ATTR_NAME,sName);
+ pThor->queryPropTree(sThorMasterProcess.str())->setProp(XML_ATTR_COMPUTER, computers[i]->queryProp(XML_ATTR_NAME));
+ if (nPort != 0) pThor->queryPropTree(sThorMasterProcess.str())->setPropInt(XML_ATTR_PORT, nPort);
+ }
+ }
@jakesmith

jakesmith Oct 16, 2012

Member

It would clearer/more maintainable if you did the :

pThor->queryPropTree(sThorMasterProcess.str())

once.

Also, looks like you can common up these two branches, once you've established whether you're wanting to setup the new pProcessNode (and add it) or set properties into existing 'pMasterNode' , both can be same setProp* calls.

Gleb Aronsky HPCC-2964 Configmgr - Allow swapping of Thor Master
- From code review: Fix memory leak
- From code review: Refactor/common up code

Signed-off-by: Gleb Aronsky <gleb.aronsky@lexisnexis.com>
18eda8c
Owner

richardkchapman commented Oct 17, 2012

@jakesmith Please re-review

@jakesmith jakesmith commented on the diff Oct 17, 2012

deployment/deployutils/configenvhelper.cpp
+ IPropertyTree *pTree = (szType == NULL || strcmp(szType, XML_TAG_THORMASTERPROCESS) != 0) ? NULL : pThor->queryPropTree(sThorMasterProcess.str());
+ bool bAdd = false;
+
+ if (pTree == NULL)
+ {
+ bAdd = true;
+ pTree = createPTree(szType);
+ }
+
+ pTree->setProp(XML_ATTR_NAME,sName);
+ pTree->setProp(XML_ATTR_COMPUTER, computers[i]->queryProp(XML_ATTR_NAME));
+
+ if (nPort != 0)
+ pTree->setPropInt(XML_ATTR_PORT, nPort);
+
+ if (bAdd == true)
@jakesmith

jakesmith Oct 17, 2012

Member

should be just if (bAdd) really.

Member

jakesmith commented Oct 17, 2012

Looks ok.

@richardkchapman richardkchapman added a commit that referenced this pull request Oct 17, 2012

@richardkchapman richardkchapman Merge pull request #3517 from garonsky/issue-2964_Change_thor_master_…
…without_delete

HPCC-2964 Configmgr - Allow swapping of Thor Master

Reviewed-By: Jake Smith <jake.smith@lexisnexis.com>
9362daa

@richardkchapman richardkchapman merged commit 9362daa into hpcc-systems:candidate-3.10.x Oct 17, 2012

garonsky deleted the garonsky:issue-2964_Change_thor_master_without_delete branch Jun 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment