[metrics-bugs] #22141 [Metrics/metrics-lib]: Deprecate `DescriptorFile` and add relevant information to `Descriptor`

[Tor Bug Tracker & Wiki]

#22141: Deprecate `DescriptorFile` and add relevant information to `Descriptor`
---------------------------------+-----------------------------------
 Reporter:  karsten              |          Owner:  metrics-team
     Type:  enhancement          |         Status:  needs_review
 Priority:  Medium               |      Milestone:  metrics-lib 1.9.0
Component:  Metrics/metrics-lib  |        Version:
 Severity:  Normal               |     Resolution:
 Keywords:                       |  Actual Points:
Parent ID:                       |         Points:
 Reviewer:                       |        Sponsor:
---------------------------------+-----------------------------------

Comment (by karsten):

 Replying to [comment:7 iwakeh]:
 > DescriptorReaderImpl became more readable.  Maybe use Files.walkFileTree
 and an extension of SimpleFileVisitor to simplify the code which currently
 is in "readDescriptorFiles"?

 Good idea!  If you don't mind, let us focus on the interface part for the
 moment, and I'll make this implementation change later on (before we
 merge).  Okay?

 > Regarding `Descriptor.getException`:  The use case here is to determine,
 if the Descriptor object is valid and decide whether to use it or drop it?
 > Then, why not add a `Descriptor.valid` method returning true or false?
 The detailed logging of exceptions could be configured if wanted in
 metrics-lib as an extra logging channel for parse exceptions (this code
 would need to be added still).

 Not quite.  The application already knows that it can't use an invalid
 descriptor, because it's not an `instanceof` whatever interface it
 expected.  It's just a `Descriptor`, but not, for example, a
 `RelayServerDescriptor`.  The application can check whether
 `getException()` returns something if it wants to log that or possibly
 even handle that exception somehow.  But it could as well ignore the
 descriptor of unknown/unexpected subinterface type.

 > About parsing "future or not" I have more questions than suggestions as
 it depends highly on the implementation.
 > What do you have in mind?
 > It would also introduce some overhead regarding the cancelling, how
 should that be handled?
 > How should a client deal with the Futures it receives?

 Hmm, to be honest, I don't really know.  Maybe we can postpone that
 discussion, even at the risk of having to wait for 3.0.0.

 > BTW DescriptorImpl: The `NoSuchAlgorithmException` should be escalated
 as runtime exceptions, b/c this is a problem with the jdk/jvm and
 shouldn't be hidden away.  I'd suggest using a private method for the two
 digest calls (please find a suggestion
 [https://gitweb.torproject.org/user/iwakeh/metrics-
 lib.git/commit/?h=task-22141&id=31a2f15bdc5933cc1f240cff107e01e0fce69a8d
 here]).

 Good idea.  As stated above, I'll do this as soon as we have an interface
 we like.

 I just had a related thought on #22196 which I'll post there in the next
 hour or so.  That thought might affect what we're doing here, so let's
 briefly switch over there and return here after that. :)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22141#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online