Opened 3 years ago

Closed 19 months ago

#19640 closed enhancement (invalid)

Review and improve interface hierarchy

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

Description

see #19398 (comment 10)

Child Tickets

Change History (21)

comment:2 Changed 3 years ago by iwakeh

Milestone: metrics-lib 1.4.0

comment:3 Changed 3 years ago by karsten

Milestone: metrics-lib 1.4.0metrics-lib 1.5.0

Move to 1.5.0 in order to get 1.4.0 out sooner.

comment:4 Changed 3 years ago by iwakeh

ParseHelper should be split into a utility class that is part of the interface and the implementation specific part inside impl.

comment:5 Changed 3 years ago by iwakeh

Milestone: metrics-lib 1.5.0metrics-lib 2.0.0

comment:6 Changed 3 years ago by iwakeh

Type: defectenhancement

see also #17861.

comment:7 Changed 2 years ago by karsten

Owner: changed from karsten to metrics-team
Status: newassigned

I'd like us to flesh out how we'd like to improve the interface hierarchy, and I'll start by going through the ideas above.

  1. Comment 10 of #19398 says: "There shouldn't be two interfaces declaring the very same method; there should be a more structured hierarchy for the interfaces, too." I think I agree in theory, though I'd want to discuss how strictly we want to implement this principle. Here's a contrasting principle: let's avoid introducing interfaces that have the sole purpose of deduplicating methods. I think we need to find a middle ground here.
  1. The linked comment in NetworkStatusImpl seems related to possible improvements to the interface hierarchy, though I'd prefer if we address that issue after improving the interface hierarchy.
  1. Comment 4 above, splitting ParseHelper into interface and implementation seems unrelated, unless we're planning to make the interface part of the metrics-lib API. Right now it's solely an implementation helper to avoid duplicating code.
  1. #17861 is indeed something where we could improve the interface hierarchy, by having a separate interface for each descriptor type. But I'm not sure how to best implement such a new interface. See the heavy overlap between RelayNetworkStatusVote and RelayNetworkStatusConsensus. A new RelayNetworkStatusMicrodescConsensus interface following the current code practice would basically copy over the entire consensus interface and change a method here and there. We should avoid that.

In addition to those items, I came up with a few more ideas:

  1. When we added RelayServerDescriptor, RelayExtraInfoDescriptor, BridgeServerDescriptor, and BridgeExtraInfoDescriptor, we left all methods in the superinterfaces ServerDescriptor and ExtraInfoDescriptor. We should consider moving methods that are specific to relays or bridges to the subinterfaces.
  1. We're still using a single NetworkStatusEntry for entries in all the different network statuses. We should consider using an interface hierarchy there, too, to only provide relevant methods depending on the network status at hand. I started working on a patch, but I'd like to keep this discussion on the conceptual level for now.

And here are some more concrete suggestions for improving the interface hierarchy, somewhat overlapping with the ideas above:

  1. Introduce a NetworkStatus interface to capture all common parts of network statuses in directory protocol versions 2 and 3, including all RelayNetworkStatus* and BridgeNetworkStatus.
  1. Introduce another interface called NetworkStatusVote for common parts of network statuses in directory protocol version 3, following the common part in URLs like http://<hostname>/tor/status-vote/next/authority.z. Though there's the risk that this interface will be confused with RelayNetworkStatusVote. Another possible interface name could be RelayNetworkStatusVersion3, though I'm not a big fan of including numbers in type names.
  1. Introduce a new interface NetworkStatus.Entry just like ExitList.Entry and a set of subinterfaces like RelayNetworkStatusConsensus.Entry to address issue 6 above. Requires some heavy generics lifting.
  1. Use RelayNetworkStatusConsensus for the common parts in consensuses of any flavor and introduce RelayNetworkStatusNsConsensus for unflavored/ns-flavored and RelayNetworkStatusMicrodescConsensus for microdesc-flavored consensuses. That's basically what we did with adding new subinterfaces to ServerDescriptor.

What else can/should we do? Or which parts should we rather not do?

comment:8 Changed 2 years ago by iwakeh

Milestone: metrics-lib 2.0.0metrics-lib 1.9.0

comment:9 Changed 2 years ago by iwakeh

Also from #21932 comments 14/15, listed for discussion:

  • avoid constant duplication, think about adding or not adding certain constants to the api (refers to DescriptorImpl.NL and ExitList.EOL)
  • add overloaded methods for newScanner, b/c the usual delimiter is "\n"
  • DescriptorImpl.setDigestXXX allow empty or null argument. This should have a check even though currently the calling methods make sure the argument is not null or empty, but when working on other tasks in the future that might not be apparent anymore and would get lost w/o a check and accompanying test.
  • Think about improving TorperfResultImpl and ExitListImpl (also in regard to delimiter use, see question 4, comment 14, #21932).

comment:10 Changed 2 years ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: assignedaccepted

comment:11 Changed 2 years ago by iwakeh

Metrics-lib parses Tor protocol related descriptors and descriptors from other sources. Shouldn't this distinction be reflected in the interface hierarchy?
Have Descriptor on top and introduce an ExternalDescriptor interface for non-Tor protocol related data sources? Maybe, even have TorDescriptor too?

              Descriptor
            /           \
    TorDescriptor     ExternalDescriptor

This would provide more freedom when for example a switch to an external parsing framework (like ANTLR) is made for the Tor related descriptors. In addition, non-Tor descriptors could be modeled more easily to the soon to come Metrics-data standards and even have their own parsing/generator.

comment:12 Changed 2 years ago by karsten

Hmm. The use cases you describe make sense for adding such marker interfaces sound plausible. But it also seems easy to introduce these interfaces and potentially difficult to deprecate and remove them if we don't want them anymore. Should we wait with adding them until we need them?

comment:13 Changed 2 years ago by iwakeh

Thanks for the input!
I'm trying to consider as many topics as possible while working on this with the aim to only introduce minimal changes that provide the most benefits.

comment:14 Changed 2 years ago by karsten

Milestone: metrics-lib 1.9.0

We decided today not to let this ticket (and #17861) delay the 1.9.0 release, and 2.0.0 won't be a good target milestone either. Taking this out of planned milestones for now.

comment:15 Changed 20 months ago by karsten

Summary: review and improve interface hierarchyReview and improve interface hierarchy

Capitalize summary.

comment:16 Changed 20 months ago by karsten

Keywords: metrics-2018 added

comment:17 Changed 20 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:18 Changed 19 months ago by iwakeh

Resolution: invalid
Status: acceptedclosed

Closing as being superseded by #24028.

comment:19 Changed 19 months ago by karsten

Resolution: invalid
Status: closedreopened

Let's not close this yet. It's true that #24028 is what we should focus on now, but that doesn't mean that all the ideas have become invalid/irrelevant. Re-opening.

comment:20 in reply to:  19 Changed 19 months ago by iwakeh

Replying to karsten:

Let's not close this yet. It's true that #24028 is what we should focus on now, but that doesn't mean that all the ideas have become invalid/irrelevant. Re-opening.

Oh, the closing action just meant to state that the discussion moved on. This ticket is explicitely referenced in #24028 as a resource. But I really want to avoid to open two discussions of one topic. Moving to the new ticket seems better because there is a better structure how to go about things than the summary of (surely valid) ideas here.

I'd rather close this again for this reason, which doesn't mean the ideas here were obsolete.

comment:21 Changed 19 months ago by karsten

Resolution: invalid
Status: reopenedclosed

Great, works for me! Re-closing.

Note: See TracTickets for help on using tickets.