HPCC-19489 STD.File.RenameLogicalFile should support OVERWRITE #11607
Conversation
|
https://track.hpccsystems.com/browse/HPCC-19489 |
|
@jakesmith please review |
| @@ -523,31 +523,63 @@ FILESERVICES_API void FILESERVICES_CALL fsSetReadOnly(ICodeContext *ctx, const c | |||
| throw error.getClear(); | |||
| } | |||
|
|
|||
|
|
|||
| FILESERVICES_API void FILESERVICES_CALL fsRenameLogicalFile(ICodeContext *ctx, const char *oldname, const char *newname) | |||
| FILESERVICES_API void FILESERVICES_CALL implementRenameLogicalFile(ICodeContext *ctx, const char *oldname, const char *newname, const bool overwrite) | |||
| { | |||
| StringBuffer lfn, nlfn; | |||
There was a problem hiding this comment.
pedantic/not new: but would be slightly clearer if lfn was olfn, or these were oldLfn/newLfn or similar.
| if (overwrite) | ||
| { | ||
| if ( distributedDirectory.exists(nlfn.str(), ctx->queryUserDescriptor(), false, false) && | ||
| distributedDirectory.exists(lfn.str(), ctx->queryUserDescriptor(), false, false)) |
There was a problem hiding this comment.
If old lfn doesn't exist, there's no point in continuing at all, with or without overwrite. (renamePhysical will fail).
So would be clearer if checked if old existed and error out before anything else.
| s.append("') done"); | ||
| WUmessage(ctx, SeverityInformation, NULL, s.str()); | ||
| AuditMessage(ctx, "DeleteLogicalFile", nlfn.str()); | ||
| } |
There was a problem hiding this comment.
lines 541 to 552 are same as code in fsDeleteLogicalFile.
should be commoned up, i.e. both call common function, or possibly for this just to calll fsDeleteLogicalFile directly.
testing/regress/ecl/fileservice.ecl
Outdated
| OUTPUT(ds, , prefix + 'renametest.d00', OVERWRITE), | ||
| OUTPUT(ds, , prefix + 'afterrename1.d00', OVERWRITE), | ||
| // Remane with overwrite allowed | ||
| File.RenameLogicalFile(prefix + 'renametest.d00', prefix + 'afterrename1.d00', true), |
There was a problem hiding this comment.
I think there may be some untested problems.
- Rename external or foreign with overwrite.
Both are disallowed and will error on rename, but the delete will have already happened..
Foreign delete should fail, so may be ok.
External delete .. I'm not sure, worth testing.
i.e. need to make sure it doesn't succeed to delete - but then fail on rename. - Rename in a transaction.
Actually fileservices.ecl doesn't have any transaction tests at the moment.
I think there may be problems with rename in particular, related to the way delete happens.
See addDelayedDelete/CDelayedDelete in dadfs.cpp
There was a problem hiding this comment.
As we discussed, I will test this delete, rename in transaction.
testing/regress/ecl/fileservice.ecl
Outdated
| File.DeleteLogicalFile(prefix + 'scope1::afterrename3.d00'), | ||
| OUTPUT(ds, , prefix + 'renametest.d00', OVERWRITE), | ||
| OUTPUT(ds, , prefix + 'afterrename1.d00', OVERWRITE), | ||
| // Remane with overwrite allowed |
testing/regress/ecl/fileservice.ecl
Outdated
| OUTPUT(ds, , prefix + 'afterrename1.d00', OVERWRITE), | ||
| // Remane with overwrite allowed | ||
| File.RenameLogicalFile(prefix + 'renametest.d00', prefix + 'afterrename1.d00', true), | ||
| File.DeleteLogicalFile(prefix + 'afterrename1.d00'), |
There was a problem hiding this comment.
Would be good to read the renamed file after the test to verify before deleting it.
|
@AttilaVamos - added some comments. |
AttilaVamos
force-pushed
the
HPCC-19489-improvement-7.2.0
branch
from
32715cb
to
6581d6d
Compare
|
@jakesmith updated, please re-review. |
|
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
|
@AttilaVamos - looks good, but 1 question. Did the "As we discussed, I will test this delete, rename in transaction." happen, is there a separate JIRA? |
|
@AttilaVamos Please squash |
|
@jakesmith I will open a JIRA or perhaps two to cover rename logical file in transaction, for foreign and external file. |
|
@AttilaVamos - please squash ready for merge and reference the new JIRA's in this issues JIRA. |
Add allowOverwrite parameter to STD.File.RenameLogicalFile Add code if overwrite is allowed to: - check source and target file existence - remove existing target file Extend fileservice.ecl to test allowOverwrite option Signed-off-by: Attila Vamos <attila.vamos@gmail.com>
AttilaVamos
force-pushed
the
HPCC-19489-improvement-7.2.0
branch
from
6581d6d
to
f69f2bb
Compare
Add allowOverwrite parameter to STD.File.RenameLogicalFile
Add code if overwrite is allowed to:
Extend fileservice.ecl to test allowOverwrite option
Signed-off-by: Attila Vamos attila.vamos@gmail.com
Type of change:
Checklist:
Testing:
Changes are tested manually.