hpcc-systems/HPCC-Platform — pull request #3743 by rengolin (2012-11-29 15:07:17+00:00)
Title: HPCC-8364 Move doRemove methods to files

Yet another controversial change. The main issue was that simple removal actions were passing through files, actions, transactions, delayed-delete, back to files again and finally on the directory.

This patch simplifies considerably this confusion and removes from directories, transactions and delayed-delete all the internal logic of removing files. There's no need for checking if the file is a super-file or not, since the "detach()" method will act accordingly, depending on which class it refers to.

In the end, deleting a file via Dir->removeEntry() and file->detach() are now synonymous, with all the logic neatly confined on files/super-files.

### Review

There are many patches, and functionality passed from each of the mentioned classes to another, so I recommend to review **the final change directly**, not each path in sequence. I'm only keeping them un-squashed for reference, and to underline the patches that are changes and those that are bug-fixes.

### Controversy 1

There is one important change, however, that need reviewing. Up until now, removePhysical had the "cluster" parameter, but that was never used (either default to NULL, or explicitly passed NULL).

Upon removing the unused parameter, I realized that it was not even being used to detach files, and the only use was when removing the physical parts, or on a very special case for which I found no real usage. This can be seen on CDistributedFile::detach() on the new lexical block that used to be the old detach.

I could have added the old "(if !cluster || (!findCluster && numClusters==1))" to it, but I couldn't find a good reason to. It looks as though that was fixing a bug created elsewhere, and as usual, I prefer to see them break and fix in the right place than propagate bad design decisions.

Please, make sure this decision is actually correct. The cluster is still being passed to the physical removal by the same means it was before, ie., by getting it from the logical name.

### Controversy 2

I found who was calling remove logical only on files: rename. Since that's an internal functionality, I've added a private method: detachLogical(). For now, it shares the flag with daregress (setting it in the properties), but it must move to an internal flag as soon as daregress::testDFS() is refactored. (comments also added).

### Tests

No functional changes expected, daregress and all regressions tests pass on all three engines.

The commits will be squashed before pulling