HADOOP-19543. [ABFS][FnsOverBlob] Remove Duplicates from Blob Endpoint Listing Across Iterations #7614
Conversation
anujmodi2021
changed the title
| return continuation; | ||
| } | ||
|
|
||
| private void filterDuplicateEntriesForBlobClient( |
| } | ||
|
|
||
| @Test | ||
| public void testDuplicateEntriesAcrossListBlobIterations() throws Exception { |
There was a problem hiding this comment.
You can add one more test where there are more than one files in the directory and max list result is 1 and multiple list status calls and verify the overall file statuses don't have any duplicate entries
There was a problem hiding this comment.
The duplicate can only happen in case of non-empty explicit directories. The store won't allow me to create duplicate paths otherwise.
What I can do is modify this test to have more than one entry. A mix of all types of entries and make sure we still don't have any duplicates.
There was a problem hiding this comment.
Modified test as above
This comment was marked as outdated.
This comment was marked as outdated.
| // In both cases, we will add this. | ||
| nameToEntryMap.put(entryName, fileStatus); | ||
| fileStatuses.add(fileStatus); | ||
| } else { |
There was a problem hiding this comment.
It can be replaced with else if instead of nested if.
| * @return the list of FileStatus objects | ||
| */ | ||
| public List<FileStatus> getFileStatusList() { | ||
| public List<VersionedFileStatus> getFileStatusList() { |
There was a problem hiding this comment.
Javadoc update is needed.
Returns list of VersionedFileStatus objects
| * @param fileStatusList the list of FileStatus objects | ||
| */ | ||
| public void setFileStatusList(final List<FileStatus> fileStatusList) { | ||
| public void setFileStatusList(final List<VersionedFileStatus> fileStatusList) { |
There was a problem hiding this comment.
Same as above, javadoc update is needed
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
anujmodi2021
changed the title
anujmodi2021
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
anujmodi2021
changed the title
|
|
||
| import org.apache.hadoop.fs.FileStatus; | ||
|
|
||
| public class ListUtils { |
There was a problem hiding this comment.
add javadocs for class and its functions
|
|
||
| public class ListUtils { | ||
|
|
||
| public static List<FileStatus> getUniqueListResult(List<FileStatus> originalList) { |
There was a problem hiding this comment.
Logic looks a bit complex to read, can be refactored. One possible solution could be
public static List getUniqueListResult(List originalList) {
if (originalList == null || originalList.isEmpty()) {
return originalList;
}
List uniqueList = new ArrayList<>();
TreeMap<String, FileStatus> currentGroupMap = new TreeMap<>();
String currentPrefix = null;
for (FileStatus fileStatus : originalList) {
String fileName = fileStatus.getPath().getName();
if (currentPrefix == null || !fileName.startsWith(currentPrefix)) {
// Start of a new group
currentPrefix = fileName;
currentGroupMap.clear();
}
if (!currentGroupMap.containsKey(fileName)) {
currentGroupMap.put(fileName, fileStatus);
uniqueList.add(fileStatus);
}
}
return uniqueList;
}
There was a problem hiding this comment.
Nice Suggestion.
Taken
| List<VersionedFileStatus> fileStatusListInCurrItr = listResponseData.getFileStatusList(); | ||
| if (fileStatusListInCurrItr != null && !fileStatusListInCurrItr.isEmpty()) { | ||
| fileStatuses.addAll(fileStatusListInCurrItr); | ||
| fileStatusList.addAll(fileStatusListInCurrItr); |
There was a problem hiding this comment.
here we are iterating over a list of VersionedFileStatus and maintaing a list of FileStatus, shouldn't the new list also be of VersionedFileStatus elements ?
There was a problem hiding this comment.
Elements are always of type VersionedFileStatus only. Just the reference type is getting changed because store.listStatus() works with FileStatus only. The final list to be returned has to be of type FileStatus. Although the objects hold by it are always VersionedFileStatus and can be type casted wherever needed.
| originalList.add(getFileStatusObject(new Path("/abc"))); | ||
| originalList.add(getFileStatusObject(new Path("/abc.bak1"))); | ||
| originalList.add(getFileStatusObject(new Path("/abc"))); | ||
| validateList( originalList, 5); |
This comment was marked as outdated.
This comment was marked as outdated.
| assertExplicitDirectoryFileStatus(fileStatuses[6], fs.makeQualified(new Path("/e"))); | ||
|
|
||
| // Assert that there are no duplicates in the returned file statuses. | ||
| for (int i = 0; i < fileStatuses.length; i++) { |
There was a problem hiding this comment.
could we use a set here instead of a double loop?
something like-
Set uniquePaths = new HashSet<>();
for (FileStatus fileStatus : fileStatuses) {
Assertions.assertThat(uniquePaths.add(fileStatus.getPath()))
.describedAs("Duplicate entries found")
.isTrue();
}
There was a problem hiding this comment.
Nice suggestion.
Taken
| public class TestListUtils { | ||
|
|
||
| @Test | ||
| public void testRemoveDuplicates() { |
| validateList(originalList, 2); | ||
| } | ||
|
|
||
| private void validateList(List<FileStatus> originalList, int expectedSize) { |
There was a problem hiding this comment.
Java doc missing for this and below method as well.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
LGTM ! |
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
============================================================
|
…t Listing Across Iterations (apache#7614) Contributed by Anuj Modi Reviewed by Anmol Asrani, Manish Bhatt, Manika Joshi Signed off by Anuj Modi<anujmodi@apache.org>
5 participants
Description of PR
JIRA: https://issues.apache.org/jira/browse/HADOOP-19543
On FNS-Blob, the List Blobs API is known to return duplicate entries for non-empty explicit directories. One entry corresponds to the directory itself, and another corresponds to the marker blob that the driver internally creates and maintains to mark that path as a directory. We already know about this behaviour, and it was handled to remove such duplicate entries from the set of entries that were returned as part of current list iterations.
Due to a possible partition split, if such duplicate entries happen to be returned in separate iterations, there is no handling on this, and the caller might get back the result with duplicate entries, as happened in this case. The logic to remove duplicates was designed before the realization of the partition split.
This PR fixes this bug
How was this patch tested?
A new test for the failing scenario was added and existing test suite was ran to validate changes across all combinations.