-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Add RESTful API for distributed load #18254
Conversation
fce7f1d
to
d682742
Compare
a1eb019
to
6dd869c
Compare
dora/core/common/src/main/java/alluxio/underfs/BaseUfsStatusIterator.java
Outdated
Show resolved
Hide resolved
2d3eb9b
to
426c1cd
Compare
/** | ||
* This base under file system status iterator is for listing files iteratively. | ||
*/ | ||
public class BaseUfsStatusIterator implements Iterator<UfsStatus> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this used for listing the statuses of files only? If so, can rename it to UfsFileStatusIterator
for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the iterator can be made of type Iterator<UfsFileStatus>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the class to UfsFileStatusIterator
. But changing the type to Iterator<UfsFileStatus>
may break too many codes. Let's still use Iterator<UfsStatus>
in this PR.
|
||
private final String mPath; | ||
|
||
private final ListOptions mOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mOptions
is unused? I think you need to at least check the recursive flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this mOptions
.
|
||
private void initQueue(String path) { | ||
try { | ||
UfsStatus[] statuses = mUfs.listStatus(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UfsStatus[] statuses = mUfs.listStatus(path); | |
UfsStatus[] statuses = mUfs.listStatus(path, mOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do this becasuse we don't want it to listStatus recursively. Otherwise the feature partial list is not implemented.
return null; | ||
} | ||
return Iterators.forArray(result); | ||
return new BaseUfsStatusIterator(this, path, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contract of listStatusIterable
requires:
* @return An iterator of ufs status. Returns
* {@code null} if this abstract pathname does not denote a directory.
So you need to check if path
is a file, and return null
in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
dora/core/server/worker/src/main/java/alluxio/worker/http/HttpServerHandler.java
Outdated
Show resolved
Hide resolved
private HttpLoadOptions() { | ||
} | ||
|
||
private void setOpType(OpType opType) { | ||
mOpType = opType; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having private setters, you can put all the options as arguments to the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a builder pattern, so it would be better to put them into setters, but not constructor since this allows user to set different fields independently. Otherwise, user has to call a constructor setting all the fields at once, this breaks builder pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already have setters on HttpLoadOptions.Builder
, so you can remove the setters on the HttpLoadOptions
class.
|
||
private static final JobProgressReportFormat DEFAULT_FORMAT = JobProgressReportFormat.TEXT; | ||
|
||
private static final String JOB_TYPE = "load"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stringly typed? is there an enum for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I used enum instead.
private OpType mOpType; | ||
|
||
private boolean mPartialListing; | ||
|
||
private boolean mVerify; | ||
|
||
private long mBandWidth; | ||
|
||
private String mProgressFormat; | ||
|
||
private boolean mVerbose; | ||
|
||
private boolean mLoadMetadataOnly; | ||
|
||
private boolean mSkipIfExists; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think null
is a sensible default value for mOpType
, or 0
for mBandwidth
.
Make them Optional<>
and default to empty
, as a user may not explicitly set a value for these fields via the HTTP URL params. Let the callers decide whatever the sensible default values are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I used OptionLong
for mBandWidth
. As for opType
, user must specify the exact type, and we will check the parameter and throw exception if the parameter comes from HTTP request is not correct. So I think opType
is OK without Optional
.
fa42afc
to
38b31c9
Compare
6e53d4c
to
4ea7946
Compare
ebf3280
to
96176ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
alluxio-bot, merge this please |
The UfsFileStatusIterator implementation is incorrect given the following structure.
This would give us |
Add RESTful API for distributed load. ### Usage: **SUBMIT:** description: submit a load job example: `http://localhost:28080/v1/load?path=/&opType=submit&partialListing=false&&verify=true&bandwidth=1000&loadMetadataOnly=false&verbose=true&skipIfExists=true` **STOP:** description: stop the load job example: http://localhost:28080/v1/load?path=/&opType=stop **PROGRESS:** description: get the progress of the load job example: `http://localhost:28080/v1/load?path=/&opType=progress&progressFormat=text&verbose=true` pr-link: Alluxio#18254 change-id: cid-8a2e4f6747d7ba6cb8c25032cc13ce4ec719da8f
Add RESTful API for distributed load.
Usage:
SUBMIT:
description: submit a load job
example:
http://localhost:28080/v1/load?path=/&opType=submit&partialListing=false&&verify=true&bandwidth=1000&loadMetadataOnly=false&verbose=true&skipIfExists=true
STOP:
description: stop the load job
example:
http://localhost:28080/v1/load?path=/&opType=stop
PROGRESS:
description: get the progress of the load job
example:
http://localhost:28080/v1/load?path=/&opType=progress&progressFormat=text&verbose=true