Skip to content

Conversation

@Galsza
Copy link
Contributor

@Galsza Galsza commented May 3, 2023

What changes were proposed in this pull request?

Add a warning to the user when the list subcommand lists too few certificates that they should increase the batch size.

What is the link to the Apache JIRA

HDDS-8514

How was this patch tested?

Github actions show no test failures and tried out the command on a local cluster.

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

@Galsza , Thanks for working on this, Please find few comments.

Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

Via the current protocol design it is not possible to get the overall count of the certs, we should consider to extend the protocol response with the number of certs in the RocksDB column family, as it would be still useful to show the users the actual number of certs.

In order to do so, you can add an optional field in the SCMListCertificateResponseProto defined in SCMServerSecurityProtocol.proto, then change the implementation of the listCertificates, and return the overal count not just the list of fetched certs, so you can use that in line 94 as CertCount value, and you can give a more precise message at the end.
The current final log message is fine (after adding batch size to the message) for the case when the value is not present in the response (new client queries old server).

The non-plus-ultra would be to provide the command that produces the next batch using the -s and -c as the last line of the output.

Remove unused variable, add batchsize to increased text
@Galsza
Copy link
Contributor Author

Galsza commented May 4, 2023

As discussed in DM, the method for the number of certs in the RocksDB column family has 2 problems: it's only giving back an estimated count and for OM certs it will cause problems. It's not worth trying to use it this way.

@fapifta
Copy link
Contributor

fapifta commented May 4, 2023

Yes, we agreed that it might cause more trouble than it solves. Thank you for updating the patch.

+1, pending CI.

@ChenSammi ChenSammi merged commit ec62011 into apache:master May 8, 2023
@ChenSammi
Copy link
Contributor

Thanks @Galsza improving the message, and @fapifta for the code review.

swamirishi pushed a commit to swamirishi/ozone that referenced this pull request Jun 10, 2024
… limits (apache#4646)

Change-Id: Ie816fda22c94372a794fad9aa04f6246ca8ac674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants