Skip to content

add deleting all types of nodes #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
anhou merged 4 commits into from
Jan 7, 2016
Merged

add deleting all types of nodes #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e3f87f
add deleting all types of nodes
iceiilin Dec 29, 2015
3422b8f
Rename downstream node to target node in node deletion
iceiilin Dec 30, 2015
684b50b
use relation class to determine the operation of deleting node
iceiilin Dec 31, 2015
99d4827
Fix jsdoc and typo about node deletion
iceiilin Jan 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 138 additions & 50 deletions lib/services/nodes-api-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ di.annotate(nodeApiServiceFactory,
'Errors',
'Logger',
'_',
'Promise'
'Promise',
'Constants'
)
);
function nodeApiServiceFactory(
Expand All @@ -22,89 +23,181 @@ function nodeApiServiceFactory(
Errors,
Logger,
_,
Promise
Promise,
Constants
) {
var logger = Logger.initialize(nodeApiServiceFactory);

function NodeApiService() {
}

/**
* Find Enclosure nodes which enclose compute node
* Find target nodes that relate with the removing node
* @param {Object} node
* @return {Promise}
* @param {String} type
* @return {Promise} array of target nodes
*/
NodeApiService.prototype._findEnclNodes = function(node) {
// Find the enclosure nodes who enclose this compute node
NodeApiService.prototype._findTargetNodes = function(node, type) {
if (!_.has(node, 'relations')) {
return Promise.resolve();
return Promise.resolve([]);
}

var relation = _.find(node.relations, { relationType: 'enclosedBy' });
var relation = _.find(node.relations, { relationType: type });
if (!relation || !_.has(relation, 'targets') ) {
return Promise.resolve();
return Promise.resolve([]);
}

return Promise.map(relation.targets, function (enclNodeId) {
return waterline.nodes.needByIdentifier(enclNodeId)
return Promise.map(relation.targets, function (targetNodeId) {
return waterline.nodes.needByIdentifier(targetNodeId)
.catch(function (err) {
logger.warning("Error Getting Enclosure Node", { error: err });
logger.warning("Error getting target node with type " + type,
{ error: err });
return;
});
});
};

/**
* Remove the relations between node and enclosure node, if enclosure node
* doesn't enclose any nodes, this enclosure node is also removed.
* @param {Object} node
* @param {Array} enclNodes
* @return {Promise}
* Remove the relations to original node in target node. If the node is invalid
* or doesn't have required relation, this function doesn't need to update the
* node info and ignore silently with Promise.resolve(). If the relation field
* is empty after removing, delete this field.
* @param {Object} node node whose relation needs to be updated
* @param {String} type relation type that needs to be updated
* @param {String} targetId id in relation type that needs to be deleted
* @return {Promise} node after removing relation
*/
NodeApiService.prototype._removeEnclRelations = function(node, enclNodes) {
if (!enclNodes) {
NodeApiService.prototype._removeRelation = function(node, type, targetId) {

if (!node) {
Copy link
Member

@heckj heckj Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to resolve an empty promise here if node isn't specified, or throw an error? I think if the node isn't defined, or if the node doesn't have any relations, you may be better off being explicit about throwing an error. It'll make this function (and those composed from them) much easier to test.

Copy link
Member

@heckj heckj Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that, or update the description of this function/method to make it clear you're intending that a failure to find/remove the relation specified will result in a Promise with an empty value as opposed to an explicit failure/throw

Copy link
Member Author

@iceiilin iceiilin Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I incline to remain it as Promise.resolve(). There is possibility that some node doesn't have desired relations. This function will not update mongoDB and ignore it silently. I will update the function description.

Copy link
Contributor

@benbp benbp Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see some drawbacks to both approaches:

If we enforce throwing an error, it means that we can never repair hung states through the API (e.g. if a target relation is deleted for some reason, we can now never successfully delete other nodes with a link relation to it).

If we don't throw an error, there could be cases where an intermittent connectivity failure to the database causes us to only partially delete relations when we really meant to delete all of them.

I'm not sure which drawback we want to avoid and which we want to accept. Thoughts?

Copy link
Member Author

@iceiilin iceiilin Jan 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benbp current code doesn't hung other nodes from processing, as you mentioned in the first "if" situation, and it also uncovers the real error from database. This function (_removeRelation) throws an error when waterline.nodes.updateByIdentifier() failes; the previous actions, that always return Promise.resolve() when it don't have valid targets, is only about some logic check and don't touch asynchronous process.

return Promise.resolve();
}

// Remove the relationship between enclosure and compute node
return Promise.map(enclNodes, function(enclNode) {
if (!_.has(enclNode, 'relations')) {
return;
}
// Remove the relationship between node and downstram node
if (!_.has(node, 'relations')) {
return Promise.resolve();
}

var index = _.findIndex(enclNode.relations, { relationType: 'encloses' });
if (index === -1 || !_.has(enclNode.relations[index], 'targets')) {
return;
}
var index = _.findIndex(node.relations, { relationType: type });
if (index === -1 || !_.has(node.relations[index], 'targets')) {
return Promise.resolve();
}

if (_.indexOf(enclNode.relations[index].targets, node.id) !== -1) {
_.pull(enclNode.relations[index].targets, node.id);
// Remove target node id in relation field
if (_.indexOf(node.relations[index].targets, targetId) !== -1) {
_.pull(node.relations[index].targets, targetId);
}

// Remove the type of relation if no targets in it
if (node.relations[index].targets.length === 0) {
_.pull(node.relations, node.relations[index]);
}

return waterline.nodes.updateByIdentifier(node.id,
{relations: node.relations});
};

/**
* Handle target nodes. Update relation type field and remove the target
* node if needed.
* @param {Array} targetNodes array of target nodes
* @param {String} type relation type in the removing node
* @param {String} nodeId removing node id
* @return {Promise}
*/
NodeApiService.prototype._handleTargetNodes = function(targetNodes, type, nodeId) {
Copy link
Member

@heckj heckj Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion only: you might consider naming this _deleteTargetNodes instead of _handleTargetNodes since this method appears to have the key logic behind when a node should be deleted and if an error should be thrown (for example, when a workflow is operational on the node)

Copy link
Member Author

@iceiilin iceiilin Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that "handle" is an not explicit action. But this function deals with target nodes with different actions, like delete and update, and "handle" might cover them

var self = this;
var targetType = '';
var relationClass = null;
var needDel = false;

if (Constants.NodeRelations.hasOwnProperty(type)) {
targetType = Constants.NodeRelations[type].mapping;
relationClass = Constants.NodeRelations[type].relationClass;
}

if ((relationClass === 'component') &&
Copy link
Member

@heckj heckj Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would probably make more sense to include this logic (to delete or not) within the promise chain (around line 125) since you're starting with a single node in this function and cascading to build a map of all nodes you want to delete. If you're expecting an input of nodes, then you may want to construct a promise chain from that input (assuming the input is an array of nodes) and utilize Promise.each.

Copy link
Member Author

@iceiilin iceiilin Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand your idea correctly, you suggested to move the the needDel assignment just before dealing with every node. I would argue that this function (_handleTargetNodes) deals with all target nodes in one relation (like {relationType:"encloses", targets:['aaa', 'bbb'...]}), thus the deletion action is the same.

(type.indexOf('By') === -1)) {
// Its components need to be deleted
needDel = true;
}

return Promise.resolve()
.then(function () {
if (needDel === true) {
// Throw error when all target nodes need to be deleted
// but at least one of them have active workflow
return Promise.map(targetNodes, function(targetNode) {
Copy link
Member

@heckj heckj Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the intent that if ANY of the nodes in the target list aren't ready to be deleted, that this entire call should "fail"?

Copy link
Member Author

@iceiilin iceiilin Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The node that user requires to delete will be removed only after all its target nodes has been deleted (if their relation requires) successfully.

return self._delValidityCheck(targetNode.id);
});
}
// If Enclosure node doesn't have enclosed nodes
// remove this enclosure node, else remove relationships
if (enclNode.relations[index].targets.length === 0) {
return waterline.nodes.destroy({ id: enclNode.id });
} else {
return waterline.nodes.updateByIdentifier(
enclNode.id, { relations : enclNode.relations });
})
.then(function () {
return Promise.map(targetNodes, function (targetNode) {
// Delete target nodes if it is required
if (needDel === true) {
Copy link
Member

@heckj heckj Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to remove the node and then the relation, or the relation and then the node? Or does it matter?

Copy link
Member Author

@iceiilin iceiilin Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented above, remove/update target nodes, and then the node itself. I don't want the target nodes to keep non-existing relations, if the node has been deleted, while the target nodes update fails. What's your opinion?

return self.removeNode(targetNode, targetType);
}

// Update relations in the target node
return self._removeRelation(targetNode, targetType, nodeId)
.then(function (targetNode) {
if (targetNode) {
logger.debug('node updated', {id: targetNode.id, relation: targetType});
}
});
});
});
};

/**
* Check whether a node is valid to be deleted
* @param {String} nodeId
* @return {Promise}
*/
NodeApiService.prototype._delValidityCheck = function(nodeId) {
return taskGraphProtocol.getActiveTaskGraph( { target: nodeId })
.then(function (graph) {
if (graph) {
// If there is active workflow, the node cannot be deleted
return Promise.reject('Could not remove node ' + nodeId +
', active workflow is running');
}
Copy link
Contributor

@yyscamper yyscamper Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be "return Promise.rejects(new Errors.BadRequestError('xxxx');

return Promise.resolve();
});
};

/**
* Remove node related data and remove its relations with other nodes
* @param {Object} node
* @param {String} srcType
* @return {Promise}
Copy link
Contributor

@yyscamper yyscamper Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still JSDoc error, node should be String, and miss the param "srcType"

Copy link
Member

@heckj heckj Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended to take in a node object, or a nodeID string?

Copy link
Member Author

@iceiilin iceiilin Jan 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node object is needed here

*/
NodeApiService.prototype.removeNode = function(node) {
NodeApiService.prototype.removeNode = function(node, srcType) {
var self = this;

return taskGraphProtocol.getActiveTaskGraph( { target: node.id })
.then(function (graph) {
if (graph) {
throw new Errors.BadRequestError('Could not remove node ' + node.id +
', active workflow is running');
return self._delValidityCheck(node.id)
.then(function () {
if (!node.hasOwnProperty('relations')) {
return Promise.resolve();
}

return Promise.map(node.relations, function(relation) {

var type = relation.relationType;

// Skip handling relationType that comes from the upstream node
// to avoid deleting upstream nodes more than once
if (srcType && (srcType === type)) {
return Promise.resolve();
}

// Otherwise update targets node in its "relationType"
return self._findTargetNodes(node, type)
.then(function (targetNodes) {
return self._handleTargetNodes(targetNodes, type, node.id);
});
});
})
.then(function () {
return Promise.settle([
Expand All @@ -115,15 +208,10 @@ function nodeApiServiceFactory(
waterline.nodes.destroy({ id: node.id }),
waterline.catalogs.destroy({ node: node.id }),
waterline.workitems.destroy({ node: node.id })
]);
})
.then(function () {
return self._findEnclNodes(node);
})
.then(function (enclNodes) {
return self._removeEnclRelations(node, enclNodes);
]);
})
.then(function () {
logger.debug('node deleted', {id: node.id, type: node.type});
return node;
});
};
Expand Down
Loading