Prechádzať zdrojové kódy

HPCC-15199 Setting useTreeCopy may cause corrupt files on Roxie

The useTreeCopy option has never been tested in anger as far as I know (it's
been in the system for years, but no-one knows how the code works or what it
is supposed to do).

Give that the one time someone DID try it it seems to have not handled error
cases well and ended up with corrupted indexes on Roxie. the commit removes
the option altogether. The associated code is removed in 6.0.0 via a separate
pull request.

Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>
Richard Chapman 9 rokov pred
rodič
commit
6fc666a7e8
4 zmenil súbory, kde vykonal 9 pridanie a 33 odobranie
  1. 0 1
      roxie/ccd/ccd.hpp
  2. 9 25
      roxie/ccd/ccdfile.cpp
  3. 0 2
      roxie/ccd/ccdmain.cpp
  4. 0 5
      roxie/ccd/ccdstate.cpp

+ 0 - 1
roxie/ccd/ccd.hpp

@@ -392,7 +392,6 @@ extern bool probeAllRows;
 extern bool steppingEnabled;
 extern bool steppingEnabled;
 extern bool simpleLocalKeyedJoins;
 extern bool simpleLocalKeyedJoins;
 extern bool enableKeyDiff;
 extern bool enableKeyDiff;
-extern bool useTreeCopy;
 extern PTreeReaderOptions defaultXmlReadFlags;
 extern PTreeReaderOptions defaultXmlReadFlags;
 extern bool mergeSlaveStatistics;
 extern bool mergeSlaveStatistics;
 extern bool roxieMulticastEnabled;   // enable use of multicast for sending requests to slaves
 extern bool roxieMulticastEnabled;   // enable use of multicast for sending requests to slaves

+ 9 - 25
roxie/ccd/ccdfile.cpp

@@ -742,11 +742,7 @@ class CRoxieFileCache : public CInterface, implements ICopyFileProgress, impleme
             StringBuffer destPath;
             StringBuffer destPath;
             StringBuffer prevTempFile;
             StringBuffer prevTempFile;
             splitFilename(targetFilename, &destPath, &destPath, &prevTempFile, &prevTempFile);
             splitFilename(targetFilename, &destPath, &destPath, &prevTempFile, &prevTempFile);
-
-            if (useTreeCopy)
-                prevTempFile.append("*.tmp");
-            else
-                prevTempFile.append("*.$$$");
+            prevTempFile.append("*.$$$");
 
 
             Owned<IFile> dirf = createIFile(destPath.str());
             Owned<IFile> dirf = createIFile(destPath.str());
             Owned<IDirectoryIterator> iter = dirf->directoryFiles(prevTempFile.str(),false,false);
             Owned<IDirectoryIterator> iter = dirf->directoryFiles(prevTempFile.str(),false,false);
@@ -800,7 +796,7 @@ class CRoxieFileCache : public CInterface, implements ICopyFileProgress, impleme
         {
         {
             IpSubNet subnet; // preferred set but not required
             IpSubNet subnet; // preferred set but not required
             IpAddress fromip; // returned
             IpAddress fromip; // returned
-            Owned<IFile> destFile = createIFile(useTreeCopy?targetFilename:tempFile);
+            Owned<IFile> destFile = createIFile(tempFile);
 
 
             bool hardLinkCreated = false;
             bool hardLinkCreated = false;
             unsigned start = msTick();
             unsigned start = msTick();
@@ -819,17 +815,11 @@ class CRoxieFileCache : public CInterface, implements ICopyFileProgress, impleme
                         StringBuffer str;
                         StringBuffer str;
                         str.appendf("doCopyFile %s", sourceFile->queryFilename());
                         str.appendf("doCopyFile %s", sourceFile->queryFilename());
                         TimeSection timing(str.str());
                         TimeSection timing(str.str());
-                        if (useTreeCopy)
-                            sourceFile->treeCopyTo(destFile, subnet, fromip, true, copyFlags);
-                        else
-                            sourceFile->copyTo(destFile,DEFAULT_COPY_BLKSIZE,NULL,false,copyFlags);
+                        sourceFile->copyTo(destFile,DEFAULT_COPY_BLKSIZE,NULL,false,copyFlags);
                     }
                     }
                     else
                     else
                     {
                     {
-                        if (useTreeCopy)
-                            sourceFile->treeCopyTo(destFile, subnet, fromip, true, copyFlags);
-                        else
-                            sourceFile->copyTo(destFile,DEFAULT_COPY_BLKSIZE,NULL,false,copyFlags);
+                        sourceFile->copyTo(destFile,DEFAULT_COPY_BLKSIZE,NULL,false,copyFlags);
                     }
                     }
                 }
                 }
                 f->setCopying(false);
                 f->setCopying(false);
@@ -838,26 +828,20 @@ class CRoxieFileCache : public CInterface, implements ICopyFileProgress, impleme
             catch(IException *E)
             catch(IException *E)
             {
             {
                 f->setCopying(false);
                 f->setCopying(false);
-                if (!useTreeCopy)
-                { // done by tree copy
-                    EXCLOG(E, "Copy exception - remove templocal");
-                    destFile->remove(); 
-                }
+                EXCLOG(E, "Copy exception - remove templocal");
+                destFile->remove();
                 deleteTempFiles(targetFilename);
                 deleteTempFiles(targetFilename);
                 throw;
                 throw;
             }
             }
             catch(...)
             catch(...)
             {
             {
                 f->setCopying(false);
                 f->setCopying(false);
-                if (!useTreeCopy)
-                { // done by tree copy
-                    DBGLOG("%s exception - remove templocal", msg);
-                    destFile->remove(); 
-                }
+                DBGLOG("%s exception - remove templocal", msg);
+                destFile->remove();
                 deleteTempFiles(targetFilename);
                 deleteTempFiles(targetFilename);
                 throw;
                 throw;
             }
             }
-            if (!hardLinkCreated && !useTreeCopy)  // for hardlinks / treeCopy - no rename needed
+            if (!hardLinkCreated)  // for hardlinks no rename needed
             {
             {
                 try
                 try
                 {
                 {

+ 0 - 2
roxie/ccd/ccdmain.cpp

@@ -78,7 +78,6 @@ unsigned restarts = 0;
 bool heapSort = false;
 bool heapSort = false;
 bool insertionSort = false;
 bool insertionSort = false;
 bool fieldTranslationEnabled = false;
 bool fieldTranslationEnabled = false;
-bool useTreeCopy = true;
 bool mergeSlaveStatistics = true;
 bool mergeSlaveStatistics = true;
 PTreeReaderOptions defaultXmlReadFlags = ptr_ignoreWhiteSpace;
 PTreeReaderOptions defaultXmlReadFlags = ptr_ignoreWhiteSpace;
 bool runOnce = false;
 bool runOnce = false;
@@ -785,7 +784,6 @@ int STARTQUERY_API start_query(int argc, const char *argv[])
         trapTooManyActiveQueries = topology->getPropBool("@trapTooManyActiveQueries", true);
         trapTooManyActiveQueries = topology->getPropBool("@trapTooManyActiveQueries", true);
         maxEmptyLoopIterations = topology->getPropInt("@maxEmptyLoopIterations", 1000);
         maxEmptyLoopIterations = topology->getPropInt("@maxEmptyLoopIterations", 1000);
         maxGraphLoopIterations = topology->getPropInt("@maxGraphLoopIterations", 1000);
         maxGraphLoopIterations = topology->getPropInt("@maxGraphLoopIterations", 1000);
-        useTreeCopy = topology->getPropBool("@useTreeCopy", true);
         mergeSlaveStatistics = topology->getPropBool("@mergeSlaveStatistics", true);
         mergeSlaveStatistics = topology->getPropBool("@mergeSlaveStatistics", true);
 
 
         enableKeyDiff = topology->getPropBool("@enableKeyDiff", true);
         enableKeyDiff = topology->getPropBool("@enableKeyDiff", true);

+ 0 - 5
roxie/ccd/ccdstate.cpp

@@ -2782,11 +2782,6 @@ private:
                 else
                 else
                     throw MakeStringException(ROXIE_MISSING_PARAMS, "Metric name or regex missing");
                     throw MakeStringException(ROXIE_MISSING_PARAMS, "Metric name or regex missing");
             }
             }
-            else if (stricmp(queryName, "control:useTreeCopy")==0)
-            {
-                useTreeCopy = control->getPropBool("@val", true);
-                topology->setPropInt("@useTreeCopy", useTreeCopy);
-            }
             else
             else
                 unknown = true;
                 unknown = true;
             break;
             break;