HDDS-3972. Add option to limit number of items displaying through ldb tool.

[sadanand48]

What changes were proposed in this pull request?

The change here is to add an option to the ldb tool scan command to limit number of displayed items.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3972

How was this patch tested?

Added Unit Tests for the tool.

[elek]

Thanks @sadanand48 the patch, overall it looks good to me.

I appreciate that you added a unit test (big +1), but I would prefer to keep it simple as a real unit test (without starting real MiniOzoneCluster)

[elek]

MiniOzoneCluster is very heavy weight for this kind of testing. It's very slow (requires to start all the services) and easier to be flaky.

I would suggest to use pure rocksdb creation here (for example use pure DBStoreBuilder and create a pure rocksdb dir). Or if you prefer to test SCM specific rocksdb, you can use SCMMetadataStoreImpl

[sadanand48]

I have used DBStoreBuilder now. But i had to create a db with the same name 'om.db' as the tool only reads from those DB whose DBDefinition is defined. I'm not sure whether that would be a problem.

[sadanand48]

Thanks @elek for the review. Addressed Review Comments.

[elek]

Thanks the update @sadanand48. Yes it looks better (and significant better) for me.

I realized two other NITs in the test.

1. @Test(expected = IllegalArgumentException.class)

When you use complex test cases (which multiple assertions) it's not safe to use the expected attribute. Actually this test passes even if the method always returns with IllegalArgumentException as the remaining asserstions will be ignored in this case.

For example the last line, is ignored in the current patch.

    Assert.assertEquals(0, keyNames.size());

It's never called.

I think it's more safe to check if that specific call throws an exception or not:

    try {
      getKeyNames(dbScanner, "keyTable", keyNames);
      Assert.fail("Illegal argument exception is expected");
    } catch (IllegalArgumentException ex) {

    }

2. Is more minor, but the readability can be improved with using

    List<String> keyNames = getKeyNames(dbScanner, "keyTable");

instead of

getKeyNames(dbScanner, "keyTable", keyNames);

I know that it's possible to pass keyNames as reference and reset and modify the content, but the behavior seems to be more obvious (easier to read) if you just return with the selected values.

[elek]

+1 Thank you very much the update @sadanand48 (and sorry for late answer).

I am merging it now.