Skip to content
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

HDDS-3947: Sort DNs for client when the key is a file for #getFileStatus #listStatus APIs #1385

Merged
merged 5 commits into from
Sep 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,7 @@ public OzoneFileStatus getOzoneFileStatus(String volumeName,
.setBucketName(bucketName)
.setKeyName(keyName)
.setRefreshPipeline(true)
.setSortDatanodesInPipeline(topologyAwareReadEnabled)
.build();
return ozoneManagerClient.getFileStatus(keyArgs);
}
Expand Down Expand Up @@ -1112,6 +1113,7 @@ public List<OzoneFileStatus> listStatus(String volumeName, String bucketName,
.setBucketName(bucketName)
.setKeyName(keyName)
.setRefreshPipeline(true)
.setSortDatanodesInPipeline(topologyAwareReadEnabled)
.build();
return ozoneManagerClient
.listStatus(keyArgs, recursive, startKey, numEntries);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1177,6 +1177,7 @@ public OzoneFileStatus getFileStatus(OmKeyArgs args) throws IOException {
.setVolumeName(args.getVolumeName())
.setBucketName(args.getBucketName())
.setKeyName(args.getKeyName())
.setSortDatanodes(args.getSortDatanodes())
.build();
GetFileStatusRequest req =
GetFileStatusRequest.newBuilder()
Expand Down Expand Up @@ -1390,6 +1391,7 @@ public List<OzoneFileStatus> listStatus(OmKeyArgs args, boolean recursive,
.setVolumeName(args.getVolumeName())
.setBucketName(args.getBucketName())
.setKeyName(args.getKeyName())
.setSortDatanodes(args.getSortDatanodes())
.build();
ListStatusRequest listStatusRequest =
ListStatusRequest.newBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1095,8 +1095,7 @@ public void testListStatus() throws IOException {
tempFileStatus = keyManager.listStatus(dirArgs, false,
tempFileStatus != null ?
tempFileStatus.get(tempFileStatus.size() - 1).getKeyInfo()
.getKeyName() : null,
2);
.getKeyName() : null, 2);
tmpStatusSet.addAll(tempFileStatus);
} while (tempFileStatus.size() == 2);
verifyFileStatus(directory, new ArrayList<>(tmpStatusSet), directorySet,
Expand All @@ -1112,9 +1111,7 @@ public void testListStatus() throws IOException {
tempFileStatus = keyManager.listStatus(dirArgs, true,
tempFileStatus != null ?
tempFileStatus.get(tempFileStatus.size() - 1).getKeyInfo()
.getKeyName() :
null,
2);
.getKeyName() : null, 2);
tmpStatusSet.addAll(tempFileStatus);
} while (tempFileStatus.size() == 2);
verifyFileStatus(directory, new ArrayList<>(tmpStatusSet), directorySet,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1732,17 +1732,36 @@ private void validateOzoneObj(OzoneObj obj) throws OMException {
* @param args Key args
* @throws OMException if file does not exist
* if bucket does not exist
* if volume does not exist
* @throws IOException if there is error in the db
* invalid arguments
*/
public OzoneFileStatus getFileStatus(OmKeyArgs args) throws IOException {
Preconditions.checkNotNull(args, "Key args can not be null");
return getFileStatus(args, null);
}

/**
* OzoneFS api to get file status for an entry.
*
* @param args Key args
* @param clientAddress a hint to key manager, order the datanode in returned
* pipeline by distance between client and datanode.
* @throws OMException if file does not exist
* if bucket does not exist
* if volume does not exist
* @throws IOException if there is error in the db
* invalid arguments
*/
public OzoneFileStatus getFileStatus(OmKeyArgs args, String clientAddress)
throws IOException {
Preconditions.checkNotNull(args, "Key args can not be null");
String volumeName = args.getVolumeName();
String bucketName = args.getBucketName();
String keyName = args.getKeyName();

return getOzoneFileStatus(volumeName, bucketName, keyName,
args.getRefreshPipeline(), false, null);
args.getRefreshPipeline(), args.getSortDatanodes(), clientAddress);
}

private OzoneFileStatus getOzoneFileStatus(String volumeName,
Expand Down Expand Up @@ -1783,6 +1802,9 @@ private OzoneFileStatus getOzoneFileStatus(String volumeName,

// if the key is a file then do refresh pipeline info in OM by asking SCM
if (fileKeyInfo != null) {
// refreshPipeline flag check has been removed as part of
// https://issues.apache.org/jira/browse/HDDS-3658.
// Please refer this jira for more details.
refreshPipeline(fileKeyInfo);
if (sortDatanodes) {
sortDatanodeInPipeline(fileKeyInfo, clientAddress);
Expand Down Expand Up @@ -2011,7 +2033,27 @@ private void listStatusFindKeyInTableCache(
* @return list of file status
*/
public List<OzoneFileStatus> listStatus(OmKeyArgs args, boolean recursive,
String startKey, long numEntries) throws IOException {
String startKey, long numEntries)
throws IOException {
return listStatus(args, recursive, startKey, numEntries, null);
}

/**
* List the status for a file or a directory and its contents.
*
* @param args Key args
* @param recursive For a directory if true all the descendants of a
* particular directory are listed
* @param startKey Key from which listing needs to start. If startKey exists
* its status is included in the final list.
* @param numEntries Number of entries to list from the start key
* @param clientAddress a hint to key manager, order the datanode in returned
* pipeline by distance between client and datanode.
* @return list of file status
*/
public List<OzoneFileStatus> listStatus(OmKeyArgs args, boolean recursive,
String startKey, long numEntries, String clientAddress)
throws IOException {
Preconditions.checkNotNull(args, "Key args can not be null");

List<OzoneFileStatus> fileStatusList = new ArrayList<>();
Expand All @@ -2027,18 +2069,18 @@ public List<OzoneFileStatus> listStatus(OmKeyArgs args, boolean recursive,
// A set to keep track of keys deleted in cache but not flushed to DB.
Set<String> deletedKeySet = new TreeSet<>();

if (Strings.isNullOrEmpty(startKey)) {
OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
if (fileStatus.isFile()) {
return Collections.singletonList(fileStatus);
}
// keyName is a directory
startKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
}

metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
bucketName);
try {
if (Strings.isNullOrEmpty(startKey)) {
OzoneFileStatus fileStatus = getFileStatus(args);
if (fileStatus.isFile()) {
return Collections.singletonList(fileStatus);
}
// keyName is a directory
startKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
}

Table keyTable = metadataManager.getKeyTable();
Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>>
cacheIter = keyTable.cacheIterator();
Expand Down Expand Up @@ -2144,10 +2186,14 @@ public List<OzoneFileStatus> listStatus(OmKeyArgs args, boolean recursive,
metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
bucketName);
}
if (args.getRefreshPipeline()) {
for(OzoneFileStatus fileStatus : fileStatusList){

for (OzoneFileStatus fileStatus : fileStatusList) {
if (args.getRefreshPipeline()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChenSammi Was this instance of getRefreshPipeline condition retained in HDDS-3658 intentionally?

refreshPipeline(fileStatus.getKeyInfo());
}
if (args.getSortDatanodes()) {
sortDatanodeInPipeline(fileStatus.getKeyInfo(), clientAddress);
}
}
return fileStatusList;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2797,7 +2797,7 @@ public OzoneFileStatus getFileStatus(OmKeyArgs args) throws IOException {

try {
metrics.incNumGetFileStatus();
return keyManager.getFileStatus(args);
return keyManager.getFileStatus(args, getClientAddress());
} catch (IOException ex) {
metrics.incNumGetFileStatusFails();
auditSuccess = false;
Expand Down Expand Up @@ -2921,7 +2921,8 @@ public List<OzoneFileStatus> listStatus(OmKeyArgs args, boolean recursive,

try {
metrics.incNumListStatus();
return keyManager.listStatus(args, recursive, startKey, numEntries);
return keyManager.listStatus(args, recursive, startKey, numEntries,
getClientAddress());
} catch (Exception ex) {
metrics.incNumListStatusFails();
auditSuccess = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,28 @@
* Ozone Manager FileSystem interface.
*/
public interface OzoneManagerFS extends IOzoneAcl {

/**
* Get file status for a file or a directory.
*
* @param args the args of the key provided by client.
* @return file status.
* @throws IOException if file or bucket or volume does not exist
*/
OzoneFileStatus getFileStatus(OmKeyArgs args) throws IOException;

/**
* Get file status for a file or a directory.
*
* @param args the args of the key provided by client.
* @param clientAddress a hint to key manager, order the datanode in returned
* pipeline by distance between client and datanode.
* @return file status.
* @throws IOException if file or bucket or volume does not exist
*/
OzoneFileStatus getFileStatus(OmKeyArgs args, String clientAddress)
throws IOException;

void createDirectory(OmKeyArgs args) throws IOException;

OpenKeySession createFile(OmKeyArgs args, boolean isOverWrite,
Expand All @@ -49,6 +69,37 @@ OpenKeySession createFile(OmKeyArgs args, boolean isOverWrite,
*/
OmKeyInfo lookupFile(OmKeyArgs args, String clientAddress) throws IOException;

/**
* List the status for a file or a directory and its contents.
*
* @param keyArgs the args of the key provided by client.
* @param recursive For a directory if true all the descendants of a
* particular directory are listed
* @param startKey Key from which listing needs to start. If startKey
* exists its status is included in the final list.
* @param numEntries Number of entries to list from the start key
* @return list of file status
* @throws IOException if file or bucket or volume does not exist
*/
List<OzoneFileStatus> listStatus(OmKeyArgs keyArgs, boolean recursive,
String startKey, long numEntries)
throws IOException;

/**
* List the status for a file or a directory and its contents.
*
* @param keyArgs the args of the key provided by client.
* @param recursive For a directory if true all the descendants of a
* particular directory are listed
* @param startKey Key from which listing needs to start. If startKey
* exists its status is included in the final list.
* @param numEntries Number of entries to list from the start key
* @param clientAddress a hint to key manager, order the datanode in returned
* pipeline by distance between client and datanode.
* @return list of file status
* @throws IOException if file or bucket or volume does not exist
*/
List<OzoneFileStatus> listStatus(OmKeyArgs keyArgs, boolean recursive,
String startKey, long numEntries) throws IOException;
String startKey, long numEntries, String clientAddress)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my comment on the previous PR:

Should we also keep the two original methods?

I meant both getFileStatus and listStatus. Sorry if it was unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will make the changes in my next commit.

Copy link
Contributor Author

@rakeshadr rakeshadr Sep 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adoroszlai , Sorry for re-opening the discussion on this comment. On a second thought listStatus is only used in one place in the source code and there won't be any other caller for listStatus() without ClientAddress. So, how about deleting the original method and can add it later if there is any need ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listStatus is only used in one place in the source code

It is also used by several tests, which all pass null as address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, addressed in my new commit

throws IOException;
}