Skip to content

HDDS-2380. Use the Table.isExist() API instead of get() API while checking for presence of key. #102

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.

Sign up for GitHub

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

Closed
avijayanhwx wants to merge 1 commit into from
Closed

HDDS-2380. Use the Table.isExist() API instead of get() API while checking for presence of key. #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ne/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,15 @@ public static OMDirectoryResult verifyFilesInPath(
String dbDirKeyName = omMetadataManager.getOzoneDirKey(volumeName,
bucketName, pathName);

if (omMetadataManager.getKeyTable().get(dbKeyName) != null) {
if (omMetadataManager.getKeyTable().isExist(dbKeyName)) {
Copy link
Contributor

@anuengineer anuengineer Oct 31, 2019

Choose a reason for hiding this comment

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

This patch looks correct, but I have a question about the earlier patch that went in to support this.

The documentation of keyMayExist -- says this.

public boolean keyMayExist(ColumnFamilyHandle columnFamilyHandle,
byte[] key,
java.lang.StringBuilder value)
If the key definitely does not exist in the database, then this method returns false, else true. This check is potentially lighter-weight than invoking DB::Get(). One way to make this lighter weight is to avoid doing any IOs.
Parameters:
columnFamilyHandle - ColumnFamilyHandle instance
key - byte array of a key to search for
value - StringBuilder instance which is a out parameter if a value is found in block-cache.

There is a value parameter in the call, which is documented as -- out parameter if the value is found in the block cache.

The objective of calling into this function, I suppose to leverage that -- Mind you the keyMayExist is not bloom filter lookup, it says If the value is in memory already you get access to it.

However, if you read the code -- it looks like we ignore the possiblity of gettting the value back from the block cache.

Here is the code that was committed via
"HDDS-1691 : RDBTable#isExist should use Rocksdb#keyMayExist"
// RocksDB#keyMayExist // If the key definitely does not exist in the database, then this // method returns false, else true. return db.keyMayExist(handle, key, new StringBuilder()) && db.get(handle, key) != null;

So how does this call eliminate the IO cost, since the new StringBuilder is completely ignored ?

Copy link
Contributor Author

@avijayanhwx avijayanhwx Oct 31, 2019

Choose a reason for hiding this comment

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

Thank you @anuengineer. This is a valid observation. I will fix the working of the isExist method in #101 where we are modifying the implementation.

Copy link
Contributor

@bharatviswa504 bharatviswa504 Oct 31, 2019

Choose a reason for hiding this comment

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

yes, thanks @anuengineer for flagging it. Yes in true case if we get value from block cache we can use that.

Copy link
Contributor

@anuengineer anuengineer Oct 31, 2019

Choose a reason for hiding this comment

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

perfect, let me go ahead and commit this patch, since you will fix that issue in the #101 pull request.

// Found a file in the given path.
// Check if this is actual file or a file in the given path
if (dbKeyName.equals(fileNameFromDetails)) {
return OMDirectoryResult.FILE_EXISTS;
} else {
return OMDirectoryResult.FILE_EXISTS_IN_GIVENPATH;
}
} else if (omMetadataManager.getKeyTable().get(dbDirKeyName) != null) {
} else if (omMetadataManager.getKeyTable().isExist(dbDirKeyName)) {
// Found a directory in the given path.
// Check if this is actual directory or a directory in the given path
if (dbDirKeyName.equals(dirNameFromDetails)) {
Expand Down
2 changes: 1 addition & 1 deletion .../src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ContainerDBServiceProviderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ public long getKeyCountForContainer(Long containerID) throws IOException {
*/
@Override
public boolean doesContainerExists(Long containerID) throws IOException {
return containerKeyCountTable.get(containerID) != null;
return containerKeyCountTable.isExist(containerID);
}

/**
Expand Down