HDDS-1986. Fix listkeys API.

[bharatviswa504]

Implement listKeys API.

[smengcl]

/label ozone

[arp7]

The second iteration is unfortunate. We should see if there is a way to avoid it.

[arp7]

How are you ensuring entries are in the cache? For that you have to pause double buffer flush right?

[bharatviswa504]

In this test case, we are adding entries in cache manually. It is not an integration test.

[arp7]

The change looks okay. I am worried about the potential performance of listKeys though. I am okay to let this go in for now since it fixes a correctness issue. However we will need to measure & fix list performance at some point.

[anuengineer]

or <= 0 ?

[arp7]

Yeah that would be a nice bit of defensive programming. Let's make the check <= 0.

[anuengineer]

How many keys are expected in this cache? and how many in the tree ?

[anuengineer]

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.

[anuengineer]

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.

[bharatviswa504]

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)

[bharatviswa504]

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)

[arp7]

+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.

[bharatviswa504]

Addressed the review comment.

[bharatviswa504]

+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.

I will run the benchmarks once after the list Operations is fixed.

[anuengineer]

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.
FYI: @elek , I know that you might not be happy with this approach. But it is hard to judge the impact of these change till we have this code in and start testing.

[bharatviswa504]

Thank You @arp7 and @anuengineer for the review.
I will try to run the benchmark and update it.

[nandakumar131]

Overall the patch looks ok.
We should definitely try to optimise the listKeys call.

[nandakumar131]

Thanks @bharatviswa504 for working on this, overall the change looks ok.

[bharatviswa504]

Thank You all for the review.
I have committed this to the trunk.