HPCC-30090 Find Dropzone plane using host and path when spraying #17676
Conversation
|
https://track.hpccsystems.com/browse/HPCC-30090 |
esp/services/ws_fs/ws_fsService.cpp
Outdated
| throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "The host '%s' defined in the SourcePath '%s' does not match with the host '%s' defined in SourceIP.", hostInPath.str(), file, hostReq); | ||
| } | ||
| if (isAbsolutePath(firstFilePath)) | ||
| return findDropZonePlane(firstFilePath, hostReq, true, isContainerized()); |
There was a problem hiding this comment.
unless I'm missing something this doesn't look right..
'file' will be e.g. /mydropzone/filename.csv
so it will be calling findDropZonePlane(path="/mydropzone/filename.csv", ... )
which will not match a plane configured with /mydropzone
i.e. I think you need to strip the "/filename.csv" and test only with the path
There was a problem hiding this comment.
I know it's not supposed to / not supported, if they have multiple filenames that point to different planes, but it would be validating that here, i.e. don't just return 1st match. Instead retain it, and check other paths match, or if a UNC, both the host and the path match - fire an error if any don't (unsupported).
There was a problem hiding this comment.
I found that the filename.csv does not have to be stripped because the findDropZonePlane() will call the isPathInPlane() to validate the path:
bool isPathInPlane(IPropertyTree *plane, const char *path)
{
return isEmptyString(path) || startsWith(path, plane->queryProp("@Prefix"));
}
There was a problem hiding this comment.
I know it's not supposed to / not supported, if they have multiple filenames that point to different planes, but it would be validating that here, i.e. don't just return 1st match. Instead retain it, and check other paths match, or if a UNC, both the host and the path match - fire an error if any don't (unsupported).
The existing code does check other paths and host in line 2001 to 2039. Do we want to duplicate the function or refactor them in this PR?
There was a problem hiding this comment.
I found that the filename.csv does not have to be stripped because the findDropZonePlane() will call the isPathInPlane() to validate the path: bool isPathInPlane(IPropertyTree *plane, const char *path) { return isEmptyString(path) || startsWith(path, plane->queryProp("@Prefix")); }
ok, yes agree, I misinterpreted what isPlaneInPath was doing. It's fine as it is.
There was a problem hiding this comment.
Lookup based on the 1st file, if it is relative, then it has to lookup by host only.
What if >1 dropzones have the same host (with different paths) and the 2nd filename has an absolute path which matches with the path of the 2nd dropzone?
There was a problem hiding this comment.
all inconsistencies should be rejected.
Find the dropzone based on the 1st file + IP from UNC, or IP from request, and if any subsequent ones do not lie in that dropzone, throw an error.
There was a problem hiding this comment.
For bare-metal and isDropZoneRestrictionEnabled()==false, the findDropZonePlane() may return nullptr. I guess the code should call the findDropZonePlane() if it is 1st file and !dropZone?
|
@jakesmith please see my responses to your comments. |
|
@jakesmith please review my new commit. |
wangkx
force-pushed
the
h30090
branch
2 times, most recently
from
8c2dc95
to
1d6c72d
Compare
|
@jakesmith please review my new commit. |
There was a problem hiding this comment.
@wangkx - looks good, 1 minor comment, but (not new), where is isDropZoneRestrictionEnabled validated? It looks like it will let it proceed if a dropzone is not found, even if isDropZoneRestrictionEnabled are on?
I see that getAndValidateDropZone uses isDropZoneRestrictionEnabled, but that's not called in this context, should it be?
esp/services/ws_fs/ws_fsService.cpp
Outdated
| throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Path '%s' is not valid for dropzone '%s'", path, sourcePlaneReq.str()); | ||
| } | ||
| else | ||
| { | ||
| if (!dropZone && (i == 0)) | ||
| { | ||
| dropZone.setown(findDropZonePlane(nullptr, sourceIPReq, true, isContainerized())); |
There was a problem hiding this comment.
might benefit from a comment above this line to say that if here, it sourceIPReq must have been supplied
|
@jakesmith please review my changes. |
Signed-off-by: wangkx <[email protected]>
|
@ghalliday this PR is ready to be merged. |
Type of change:
Checklist:
Smoketest:
Testing: