Opened 3 years ago

Closed 2 years ago

#20333 closed enhancement (implemented)

add descriptor digest to vote and streamline method name

Reported by: iwakeh Owned by: karsten
Priority: Medium Milestone: metrics-lib 1.7.0
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There is currently not vote descriptor digest (so CollecTor calculates it on its own). Most other descriptor types provide a digest.

In addition, it might be nice to provide a getDigest method for the different descriptor digests instead of XyzDescriptor.getXyzDigest.

Child Tickets

Change History (7)

comment:1 in reply to:  description Changed 3 years ago by karsten

Replying to iwakeh:

There is currently not vote descriptor digest (so CollecTor calculates it on its own). Most other descriptor types provide a digest.

Oh, that sounds like an oversight then. Nobody needed that yet, so it has not been implemented. Let's implement it!

In addition, it might be nice to provide a getDigest method for the different descriptor digests instead of XyzDescriptor.getXyzDigest.

What do you suggest? Just rename the method and deprecate the old one? Is that worth the possible trouble for developers who need to adapt method names? (I just remember how I had to change shaHex() to sha1Hex() the other day and found that change rather annoying.) And having two methods that do the exact same thing seems rather confusing, too.

comment:2 Changed 3 years ago by karsten

Somewhat related to the sha1Hex() example, depending on the descriptor we might need more than just one method for returning a descriptor digest: sha-1, sha-256, etc. Of course, we could pick one name per digest algorithm. But maybe we'll also need different encodings, hex vs. base64. This can get messy.

comment:3 Changed 3 years ago by iwakeh

This could get solved by overloading with an additional parameter for the algorithm.
That could also be useful for the sha*Hex methods. So, it might be good to look for other candidates.

From a developing user point of view I'd rather have these methods named the same and prefer a short name. (Just having "digest()" is worth considering, but that is a bigger change.) It's easier to make an api popular when there is a consistent and lean naming scheme.

Some examples:
relayDesc.getDigest() instead of relayDesc.getServerDescriptorDigest()
relayDesc.getDigest(Sha256) instead of relayDesc.getServerDescriptorDigest256()
micro.getDigest() instead of micro.getMicrodescriptorDigest()

Of course, there needs to be a transition and the clients' in Metrics products will have to be adapted.

comment:4 Changed 3 years ago by iwakeh

Milestone: metrics-lib 1.5.0metrics-lib 2.0.0

comment:5 Changed 2 years ago by karsten

Milestone: metrics-lib 2.0.0metrics-lib 1.7.0
Status: newneeds_review

Alright, I worked on a possible patch to move this discussion forward:

  • All methods returning the digest of a given descriptor use a common scheme, getDigestAlgEnc(), for example, getDigestSha1Hex().
  • All methods returning a digest referencing another descriptor contain a word or two explaining what other descriptor they are referencing, but they also use AlgEnc, for example, getVoteDigestSha1Hex().

I also added a digest method to votes.

I did not switch to digest(), digestAlgEnc(), or getDigest(Alg), because I found those names not in line with the other methods. Let me know if you disagree, and why, and let's discuss.

Assigning to metrics-lib 1.7.0, because this is a backward-compatible change that doesn't seem to be too hard to make.

Please review my task-20333 branch.

comment:6 Changed 2 years ago by iwakeh

Status: needs_reviewmerge_ready

Looks fine, all checks and tests pass.
The renaming makes sense; and the decision for a scheme that uses the algorithm name as argument could be revisited when there are more algorithms to choose from.

Merge ready!

comment:7 Changed 2 years ago by karsten

Resolution: implemented
Status: merge_readyclosed

Great, thanks for the review! Cherry-picked and pushed. Closing.

Note: See TracTickets for help on using tickets.