You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/02/08 05:05:21 UTC
[GitHub] [ozone] kerneltime opened a new pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
kerneltime opened a new pull request #3053:
URL: https://github.com/apache/ozone/pull/3053
## What changes were proposed in this pull request?
Remove deleted cache
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6238
## How was this patch tested?
Unit tests in CI test should exercise this code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r801931986
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1414,9 +1421,9 @@ private void listStatusFindKeyInTableCache(
}
OmKeyInfo cacheOmKeyInfo = entry.getValue().getCacheValue();
// cacheOmKeyInfo is null if an entry is deleted in cache
- if (cacheOmKeyInfo != null) {
- if (cacheKey.startsWith(startCacheKey) &&
- cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheKey.startsWith(startCacheKey) &&
+ cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheOmKeyInfo != null) {
Review comment:
At this point we already have `cacheOmKeyInfo`, comparing it to `null` is cheaper than string prefix check.
So
```java
if (cacheOmKeyInfo != null &&
cacheKey.startsWith(startCacheKey) &&
cacheKey.compareTo(startCacheKey) >= 0) {
```
is cheaper than
```java
if (cacheKey.startsWith(startCacheKey) &&
cacheKey.compareTo(startCacheKey) >= 0 &&
cacheOmKeyInfo != null) {
```
Reordering would make sense if we kept `deletedKeySet` and wanted to avoid growing that for keys that do not match the prefix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r802619661
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1519,7 +1521,7 @@ private void listStatusFindKeyInTableCache(
// First, find key in TableCache
listStatusFindKeyInTableCache(cacheIter, keyArgs, startCacheKey,
Review comment:
Do we need to preserve the sorted enumeration?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1033412352
@kerneltime We need to handle `deletedKeySet` usage in `KeyManagerImpl#listStatusFSO()` logic. In FSO, the number of entries in the cache will be pretty less compares to ObjectStore. Because in OBS a single `OMKeysDeleteRequest` contains a bunch of keys, but in FSO `OMKeyDeleteRequest` will have only one key(file/directory) entry and it is optimized one.
https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java#L1668
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r801504177
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1414,9 +1421,9 @@ private void listStatusFindKeyInTableCache(
}
OmKeyInfo cacheOmKeyInfo = entry.getValue().getCacheValue();
// cacheOmKeyInfo is null if an entry is deleted in cache
- if (cacheOmKeyInfo != null) {
- if (cacheKey.startsWith(startCacheKey) &&
- cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheKey.startsWith(startCacheKey) &&
+ cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheOmKeyInfo != null) {
Review comment:
Any reason to move this line (`if (cacheOmKeyInfo != null)`) below the prefix and compare check? This means we would be comparing keys with deleted keys and then throwing the deleted ones away. We could avoid the compare operations by discarding the deleted ones first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1035098002
Thanks @kerneltime for the patch, @rakeshadr and @sodonnel for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1032908524
`TestOzoneFileSystem` passed locally for me.. not sure why CI timed out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1033414407
> CI failure seems unrelated
>
> ```
> - Curl error (28): Timeout was reached for https://vault.centos.org/centos/8/AppStream/x86_64/os/repodata/d8472d61c5e53a3e9cbffb68e0dddbd04a07c2b7d864b07ddd211c6ad1380c6e-primary.xml.gz
> ...
> ERROR: Test execution of ozonesecure-mr is FAILED!!!!```
> ```
Unrelated indeed, being fixed in #3057.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r802615794
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1397,13 +1397,20 @@ public void refresh(OmKeyInfo key) throws IOException {
refreshPipeline(Arrays.asList(key));
}
+ private boolean isKeyDeleted(String key, Table keyTable) {
Review comment:
Will do it in the next set of changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
sodonnel commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1032481062
The original Jira is already merged and closed. We probably should attach this change to a new Jira "Improve memory profile for listStatus API call for FSO buckets" or something like that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1032875862
> The original Jira is already merged and closed. We probably should attach this change to a new Jira "Improve memory profile for listStatus API call for FSO buckets" or something like that.
I did.. I just copied the wrong URL for the PR. The commit message links to the right one https://issues.apache.org/jira/browse/HDDS-6278
Fixed it here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr edited a comment on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
rakeshadr edited a comment on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1033412352
@kerneltime
We need to handle `deletedKeySet` usage in `KeyManagerImpl#listStatusFSO()` logic as well.
In FSO, the number of entries in the cache will be pretty less compares to OBS and the overall memory foot print in FSO will be comparatively lesser than OBS. Because in OBS a single `OMKeysDeleteRequest` contains a bunch of keys, but in FSO `OMKeyDeleteRequest` will have only one key(file/directory) entry and it is optimized one.
[KeyManagerImpl.java#L1668](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java#L1668)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r802304793
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1519,7 +1521,7 @@ private void listStatusFindKeyInTableCache(
// First, find key in TableCache
listStatusFindKeyInTableCache(cacheIter, keyArgs, startCacheKey,
Review comment:
Can we initialize `int countEntries = cacheKeyMap.size();` instead of `int countEntries = 0;` ?
This will ensure only the delta will be fetched from OM DB.
Also, I think `map -> to -> list `conversion logic can be simplified because at the end `cacheKeyMap.size() <= numEntries`.
[KeyManagerImpl.java#L1596](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java#L1596)
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1397,13 +1397,20 @@ public void refresh(OmKeyInfo key) throws IOException {
refreshPipeline(Arrays.asList(key));
}
+ private boolean isKeyDeleted(String key, Table keyTable) {
Review comment:
Thanks for the meaningful method. Similar checks used in [OmMetadataManagerImpl.java#L936](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java#L936) . How about moving `#isKeyDeleted()` to a common function and reuse it. Maybe into OzoneManagerUtils or OmMetadataManager or a better.
OmMetadataManagerImpl.java#L936
```
CacheValue<OmKeyInfo> cacheValue = keyTable.getCacheValue(new CacheKey<>(kv.getKey()));
if(cacheValue == null || cacheValue.getCacheValue() != null) {
.....
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r801887955
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1414,9 +1421,9 @@ private void listStatusFindKeyInTableCache(
}
OmKeyInfo cacheOmKeyInfo = entry.getValue().getCacheValue();
// cacheOmKeyInfo is null if an entry is deleted in cache
- if (cacheOmKeyInfo != null) {
- if (cacheKey.startsWith(startCacheKey) &&
- cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheKey.startsWith(startCacheKey) &&
+ cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheOmKeyInfo != null) {
Review comment:
The question is are deleted keys more common than keys that do not match prefixes? I would guess in a normal load pattern having keys in the cache that are not in the prefix range is more common.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1033191713
CI failure seems unrelated
```
Errors during downloading metadata for repository 'AppStream':
[660](https://github.com/apache/ozone/runs/5117443339?check_suite_focus=true#step:5:660)
- Curl error (28): Timeout was reached for https://vault.centos.org/centos/8/AppStream/x86_64/os/repodata/d8472d61c5e53a3e9cbffb68e0dddbd04a07c2b7d864b07ddd211c6ad1380c6e-primary.xml.gz [Connection timed out after 30932 milliseconds]
[661](https://github.com/apache/ozone/runs/5117443339?check_suite_focus=true#step:5:661)
- Curl error (28): Timeout was reached for http://vault.centos.org/centos/8/AppStream/x86_64/os/repodata/d8472d61c5e53a3e9cbffb68e0dddbd04a07c2b7d864b07ddd211c6ad1380c6e-primary.xml.gz [Operation too slow. Less than 1000 bytes/sec transferred the last 30 seconds]
[662](https://github.com/apache/ozone/runs/5117443339?check_suite_focus=true#step:5:662)
- Curl error (28): Timeout was reached for http://vault.centos.org/centos/8/AppStream/x86_64/os/repodata/768e088faaaba73d00aee49f134e22d5d1803171ffb167260c8b55f4165e0372-filelists.xml.gz [Operation too slow. Less than 1000 bytes/sec transferred the last 30 seconds]
[663](https://github.com/apache/ozone/runs/5117443339?check_suite_focus=true#step:5:663)
- Curl error (28): Timeout was reached for http://vault.centos.org/centos/8/AppStream/x86_64/os/repodata/5ea46cc5dfdd4a6f9c181ef29daa4a386e7843080cd625843267860d444df2f3-comps-AppStream.x86_64.xml [Operation too slow. Less than 1000 bytes/sec transferred the last 30 seconds]
[664](https://github.com/apache/ozone/runs/5117443339?check_suite_focus=true#step:5:664)
Error: Failed to download metadata for repo 'AppStream': Yum repo downloading error: Downloading error(s): repodata/d8472d61c5e53a3e9cbffb68e0dddbd04a07c2b7d864b07ddd211c6ad1380c6e-primary.xml.gz - Cannot download, all mirrors were already tried without success; repodata/768e088faaaba73d00aee49f134e22d5d1803171ffb167260c8b55f4165e0372-filelists.xml.gz - Cannot download, all mirrors were already tried without success; repodata/5ea46cc5dfdd4a6f9c181ef29daa4a386e7843080cd625843267860d444df2f3-comps-AppStream.x86_64.xml - Cannot download, all mirrors were already tried without success; repodata/75e00d390273f65b94d63a5ac7db2677d832bfa95b3d7eecf651c6f45e33f8ff-modules.yaml.xz - Cannot download, all mirrors were already tried without success
[665](https://github.com/apache/ozone/runs/5117443339?check_suite_focus=true#step:5:665)
ERROR: Test execution of ozonesecure-mr is FAILED!!!!```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #3053:
URL: https://github.com/apache/ozone/pull/3053
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r801911915
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1414,9 +1421,9 @@ private void listStatusFindKeyInTableCache(
}
OmKeyInfo cacheOmKeyInfo = entry.getValue().getCacheValue();
// cacheOmKeyInfo is null if an entry is deleted in cache
- if (cacheOmKeyInfo != null) {
- if (cacheKey.startsWith(startCacheKey) &&
- cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheKey.startsWith(startCacheKey) &&
+ cacheKey.compareTo(startCacheKey) >= 0) {
+ if (cacheOmKeyInfo != null) {
Review comment:
Also, it should be cheaper to do prefix checks than look up the cache..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] rakeshadr commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r802719154
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1519,7 +1521,7 @@ private void listStatusFindKeyInTableCache(
// First, find key in TableCache
listStatusFindKeyInTableCache(cacheIter, keyArgs, startCacheKey,
Review comment:
Agreed. This logic is needed to preserve the sorted order.
_For example:_
Assume there are CachedKeys = 1000.
Now the list logic will fetch complete 1024 numEntries from om DB. Then it combines and do sorting. In worst case, it will pickup all 1000 entries from cache but only use 24 entries from om DB, which resulted in 1000 unnecessary lookup on DB. In another case, it can happen like will use only a few entries from cache and use a few keys from om DB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1034009737
@adoroszlai this should be good to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#issuecomment-1033714973
> @kerneltime
>
> We need to handle `deletedKeySet` usage in `KeyManagerImpl#listStatusFSO()` logic as well.
>
>
>
> In FSO, the number of entries in the cache will be pretty less compares to OBS and the overall memory foot print in FSO will be comparatively lesser than OBS. Because in OBS a single `OMKeysDeleteRequest` contains a bunch of keys, but in FSO `OMKeyDeleteRequest` will have only one key(file/directory) entry and it is optimized one.
>
>
>
> [KeyManagerImpl.java#L1668](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java#L1668)
Yes I saw that and have a patch but I want to keep this one simple and limited to one API. Will file a separate PR. I want to go over FSO logic in detail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #3053: HDDS-6278 Improve memory profile for listStatus API call.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #3053:
URL: https://github.com/apache/ozone/pull/3053#discussion_r802928323
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -1414,23 +1421,20 @@ private void listStatusFindKeyInTableCache(
}
OmKeyInfo cacheOmKeyInfo = entry.getValue().getCacheValue();
// cacheOmKeyInfo is null if an entry is deleted in cache
- if (cacheOmKeyInfo != null) {
- if (cacheKey.startsWith(startCacheKey) &&
- cacheKey.compareTo(startCacheKey) >= 0) {
- if (!recursive) {
- String remainingKey = StringUtils.stripEnd(cacheKey.substring(
- startCacheKey.length()), OZONE_URI_DELIMITER);
- // For non-recursive, the remaining part of key can't have '/'
- if (remainingKey.contains(OZONE_URI_DELIMITER)) {
- continue;
- }
+ if (cacheKey.startsWith(startCacheKey)
+ && cacheKey.compareTo(startCacheKey) >= 0
+ && cacheOmKeyInfo != null) {
Review comment:
I think @sodonnel's comment about the order of conditions was not addressed:
```suggestion
if (cacheOmKeyInfo != null
&& cacheKey.startsWith(startCacheKey)
&& cacheKey.compareTo(startCacheKey) >= 0) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org