-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19572. [ABFS][BugFix] Empty Page Issue on Subsequent ListBlob call with NextMarker #7698
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| public List<FileStatus> postListProcessing(String relativePath, List<FileStatus> fileStatuses, | ||
| TracingContext tracingContext, URI uri) throws AzureBlobFileSystemException { | ||
| List<FileStatus> rectifiedFileStatuses = new ArrayList<>(); | ||
| if (fileStatuses.isEmpty() && !relativePath.equals(ROOT_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.
!ROOT_PATH.equals(relativePath) for null safety
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.
Taken
| // Root Always exists as directory. It can be an empty listing. | ||
| AbfsRestOperation pathStatus = this.getPathStatus(relativePath, tracingContext, null, false); | ||
| BlobListResultSchema listResultSchema = getListResultSchemaFromPathStatus(relativePath, pathStatus); | ||
| LOG.debug("ListStatus attempted on a file path. Returning file status."); |
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.
Add path as well in the log
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.
Taken
| return listResponseData; | ||
| } | ||
|
|
||
| @Override |
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.
add javadocs
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.
Taken
| } | ||
|
|
||
|
|
||
| @Override |
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.
Java doc 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.
Taken
| public abstract ListResponseData listPath(String relativePath, boolean recursive, | ||
| int listMaxResults, String continuation, TracingContext tracingContext, URI uri) throws IOException; | ||
|
|
||
| public abstract ListResponseData listPath(String relativePath, boolean recursive, |
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.
Please add java doc for wherever needed.
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.
Taken
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java
Outdated
Show resolved
Hide resolved
...-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsBlobClient.java
Outdated
Show resolved
Hide resolved
|
:::: AGGREGATED TEST RESULT :::: ============================================================
|
|
|
||
| public ListResponseData listPath(final String relativePath, final boolean recursive, | ||
| final int listMaxResults, final String continuation, TracingContext tracingContext, URI uri, boolean is404CheckRequired) | ||
| final int listMaxResults, final String continuation, TracingContext tracingContext, URI uri) |
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 abstract listPath method in AbfsClient throws IOException, we should keep that exception same here as well.
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.
AzureBlobFileSystemException is IOException type only
| BlobListResultSchema listResultSchema = getListResultSchemaFromPathStatus(relativePath, pathStatus); | ||
| LOG.debug("ListBlob attempted on a file path. Returning file status."); | ||
| List<VersionedFileStatus> fileStatusList = new ArrayList<>(); | ||
| LOG.debug("ListStatus attempted on a file path {}. Returning file status.", relativePath); |
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.
Correction in log message GetPathStatus attempted on the file 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.
This is fine as originally ListStatus was attempted on file path.
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
bhattmanish98
left a comment
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.
+1 LGTM
|
🎊 +1 overall
This message was automatically generated. |
…call with NextMarker (apache#7698) Contributed by Anuj Modi Reviewed by Anmol Asrani, Manish Bhatt, Manika Joshi Signed off by Anuj Mod<anujmodi@apache.org>
…call with NextMarker (apache#7698) Contributed by Anuj Modi Reviewed by Anmol Asrani, Manish Bhatt, Manika Joshi Signed off by Anuj Mod<anujmodi@apache.org>
Description of PR
We came across a new behavior from the server where a ListBlob call can return an empty list even after returning a next marker(continuation token) from a previous list call.
This is to handle that case and do not imply listing to be incomplete.
How was this patch tested?
Existing Test Suite ran, and new tests are added.