Opened 4 years ago

Closed 3 years ago

#17000 closed enhancement (fixed)

Allow to distinguish between relay and bridge descriptors

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


So far, we did not distinguish between relay and bridge descriptors in the case of server descriptors and extra-info descriptors. This works just fine, because we tried hard to re-use existing keywords in relay descriptors for sanitized contents in bridge descriptors to facilitate parsing.

However, some applications process both relay and bridge descriptors and need to add workarounds for distinguishing between the two. For example, they couldn't just read the contents of CollecTor's recent/ directory, because they wouldn't be able to know whether a ServerDescriptor instance was published by a relay or bridge. Or even worse, if an application expects a given directory to contain relay descriptors and that directory suddenly contains bridge descriptors, that application wouldn't notice.

I pushed branch relay-bridge-descs to my public repository which attempts to fix this. Please review.

This change adds new interfaces to distinguish between relay and bridge descriptors. It still supports the existing two interfaces that don't make this distinction. Those two interfaces are not deprecated, because it's okay if an application does not care whether a descriptor was published by a relay or bridge.

This change is in parts based on a discussion with atagar. Thanks!

Child Tickets

Change History (5)

comment:1 Changed 4 years ago by karsten

Status: newneeds_review

comment:2 Changed 4 years ago by atagar

Hi Karsten, in general looks good! I was a little surprised at some of the code duplication. The parseDescriptors() of RelayExtraInfoDescriptorImpl verses BridgeExtraInfoDescriptorImpl are identical except for the instances it returns. Seems like something you might be able to use newInstance() for, but just a guess. Been a few years since I seriously touched java. ;)

That aside, the bridge and non-bridge classes don't really differ. I assume that means if you give a bridge annotation to a normal descriptor metrics-lib will happily parse it, and vice versa? For Stem the bridge and non-bridge classes differ, with a parent for the common bits...

comment:3 Changed 4 years ago by iwakeh

I agree with atargar's comment above.
In general it looks good, but there is some code duplication.

Was there a reason not to just extend ServerDescriptor
with methods like isRelay(), isBridge()?

In order to avoid repeated interfaces and classes for bridges and
relays in addition to the slimmed down hierarchy (as suggested by atargar)
generics could be used. Thus, one could probably also
reduce the number of existing classes and interfaces.

As an example for what I'm aiming at:

  protected static <S extends Server> List<ServerDescriptorBase<S>>
      parseDescriptors(Class<S> clazz, byte[] descriptorsBytes,
          boolean failUnrecognizedDescriptorLines)
      throws DescriptorParseException 

for the parseDescriptor method.

comment:4 Changed 4 years ago by karsten

Thanks for the reviews, atagar and iwakeh!

I do agree that we could do better with respect to avoiding unnecessary code duplication. But I'd rather want to do that for all descriptors and not just server and extra-info descriptors. I'd say let's approach that once we know how to proceed with server and extra-info descriptors here.

Regarding adding new types like RelayServerDescriptor or just adding methods like isRelay(), I slightly lean towards the former:

What I usually do when using parsed descriptors is perform a check like if (descriptor instanceof RelayServerDescriptor), which would be more complicated using these new methods:

if (descriptor instanceof ServerDescriptor) {
  ServerDescriptor serverDescriptor = (ServerDescriptor) descriptor;
  if (serverDescriptor.isRelay()) {
    // ...

Another reason is that new types allow us to add methods to either type in the future, even though we don't do that yet. That's similar to Stem's model.

So, what do you think about adding these new interfaces to the API?

By the way, related to this discussion we might want to reconsider how @type network-status-microdesc-consensus-3 are handled similar to @type network-status-consensus-3. Maybe the former deserve their own interface, RelayNetworkStatusMicrodescConsensus. That would be a new ticket though.

comment:5 Changed 3 years ago by karsten

Resolution: fixed
Severity: Normal
Status: needs_reviewclosed

Pasting a comment from atagar that he sent in private mail and that was sitting in my inbox for over two months:

I agree with you that a separate class rather than isRelay() method makes a lot more sense. As for the last bit about @type I'd need to hear more, but agree that should be its own ticket.

I went ahead and merged my earlier branch after adding a change log entry.

Regarding the (unrelated) issue about @type network-status-microdesc-consensus-3 that I brought up earlier (well, three months ago), I just created #17861.

Resolving this issue. Thanks for all the feedback!

Note: See TracTickets for help on using tickets.