HDDS-1986. Fix listkeys API. #1588
Conversation
|
/label ozone |
bharatviswa504
force-pushed
the
HDDS-1986
branch
from
8c7e306
to
f06893e
Compare
bharatviswa504
force-pushed
the
HDDS-1986
branch
2 times, most recently
from
1a83fdb
to
676e024
Compare
| // of maxKeys from the sorted map. | ||
| currentCount = 0; | ||
|
|
||
| for (Map.Entry<String, OmKeyInfo> cacheKey : cacheKeyMap.entrySet()) { |
There was a problem hiding this comment.
The second iteration is unfortunate. We should see if there is a way to avoid it.
| } | ||
|
|
||
| @Test | ||
| public void testListKeysWithFewDeleteEntriesInCache() throws Exception { |
There was a problem hiding this comment.
How are you ensuring entries are in the cache? For that you have to pause double buffer flush right?
There was a problem hiding this comment.
In this test case, we are adding entries in cache manually. It is not an integration test.
| List<OmKeyInfo> result = new ArrayList<>(); | ||
| if (maxKeys == 0) { |
There was a problem hiding this comment.
Yeah that would be a nice bit of defensive programming. Let's make the check <= 0.
| // and construct treeMap which match with keyPrefix and are greater than or | ||
| // equal to startKey. Later we can revisit this, if list operation | ||
| // is becoming slow. | ||
| while (iterator.hasNext()) { |
There was a problem hiding this comment.
How many keys are expected in this cache? and how many in the tree ?
There was a problem hiding this comment.
I feel that we are better off leaving the old code in place...where we can read from the DB.. Worst, we might have to make sure that cache is flushed to DB before doing the list operation.But practically it may not matter.
There was a problem hiding this comment.
I am ok with putting this change in if we can prove that we can do large list keys. You might want to borrow the DB from @nandakumar131 and see if you can list keys with this patch, just a thought.
There was a problem hiding this comment.
The key cache is not full cache, so if double buffer flush is going on well in background, this should have around couple of 100 entries. When I started freon with 10 threads, i see the value of maximum iteration is 200. So, almost in the cache we have 200 entries. (But on tried with busy workload clusters, slow disks)
There was a problem hiding this comment.
With current new code when the list happens we should consider entries from buffer and DB. (As we return the response to end-user after adding entries to cache). So, if user does list as next operation(next to create bucket) the bucket might/might not be there until double buffer flushes. As until double buffer flushes, we will have entries in cache. (This will not be problem for non-HA, as we return the response, only after the flush)
There was a problem hiding this comment.
+1
I am okay to get this committed with a minor comment below, assuming there are no unaddressed comments from @anuengineer.
We should benchmark list operations later in case any further optimization is needed.
|
Addressed the review comment. |
I will run the benchmarks once after the list Operations is fixed. |
|
Let us get this in, I expect some of these things we will learn the right choices only when we really benchmark and test. The sad this is that some of these changes can make our system unstable. |
|
Thank You @arp7 and @anuengineer for the review. |
bharatviswa504
force-pushed
the
HDDS-1986
branch
from
c250efb
to
d7448c3
Compare
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @bharatviswa504 for working on this, overall the change looks ok.
bharatviswa504
force-pushed
the
HDDS-1986
branch
from
d7448c3
to
5ef53de
Compare
|
Thank You all for the review. |
Implement listKeys API.