Opened 5 years ago

Closed 5 years ago

#12868 closed defect (fixed)

cyclic dependency

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

Description

One minor issue with the org.torproject.descriptor package:
the DescriptorSourceFactory in org.torproject.descriptor
creates a cycle with the org.torproject.descriptor.impl package.

(i hope i chose the right component for this ticket)

Child Tickets

Attachments (4)

metrics-lib.cycle.patch (21.0 KB) - added by iwakeh 5 years ago.
0001-Apply-iwakeh-s-12868-patch.patch (23.1 KB) - added by karsten 5 years ago.
0002-squash-Apply-iwakeh-s-12868-patch.patch (8.6 KB) - added by karsten 5 years ago.
ImplementationNotAccessibleException.java (223 bytes) - added by iwakeh 5 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by karsten

Agreed. But how would you fix this in a backward-compatible way?

If there's no such way, we'll have to deprecate the current factory but still support it for another six months or so. If we do that, I'd like to look out for more backward-incompatible changes and do the deprecation approach for all of them together.

comment:2 Changed 5 years ago by karsten

Component: Metrics Data Processormetrics-lib
Owner: set to karsten

Moving to the right component. Sorry there are so many of them.

comment:3 Changed 5 years ago by iwakeh

Why not make the decision about the actual Parser/Reader/Downloader at runtime?

The class names could be provided as properties and the DescriptorSourceFactory
loads the named classes defaulting to the current ones in case the properties are missing.

Thus, the factory doesn't need to now the impl package at compile time.

It doesn't seem like changing Parser/Reader/Downloader was intended from the beginning of metric-lib,
but might it be a good idea to provide modularization now?

comment:4 Changed 5 years ago by karsten

Plausible.

But I'd like to throw in another design option: dependency injection. I've been reading a bit about Google's Guice dependency injection framework. I like the idea, but didn't find the time to implement it yet.

Thoughts?

comment:5 Changed 5 years ago by iwakeh

Well, dependency injection is interesting, but

  • The code for using the framework might be more overhead than other solutions.
  • Using dependency injection would also affect the metric-lib clients/users. How would they be affected?


The metric-lib descriptor functionality is tied to heavy file io and url reading (afaik). In such a setting the additional abstraction level of dependency injection could be troublesome (i.e. Exception handling)?

(I sound negative here. Maybe, I am a bit of "devil's advocate" in this case.)

comment:6 Changed 5 years ago by karsten

You raise good points there. Let's go with your suggestion. Want to submit a patch?

comment:7 Changed 5 years ago by iwakeh

Status: newneeds_review

Cycles are gone

The three properties

onionoo.parser
onionoo.property
onionoo.downloader

contain the implementations for the DescriptoDownloader, DescriptorParser, and DescriptorReader implementations.

The default implementations are hard coded

LOADER_DEFAULT = "org.torproject.descriptor.impl.DescriptorDownloaderImpl";
PARSER_DEFAULT = "org.torproject.descriptor.impl.DescriptorParserImpl";
READER_DEFAULT = "org.torproject.descriptor.impl.DescriptorReaderImpl";

The default is chosen, if no property was given.
If class loading fails a runtime exception ImplementationNotAccessibleException is thrown.

One questionable thing (concerning backwards compatibility) is that I moved the DescriptorParseException up from the impl package. I think, it should be added to the api or turned into a runtime exception.

build.xml

I switched to java 7, but I also introduced a property for the java version. So it'll be easy to change.
I also included a clean task.

The patch is attached. Please review.

Changed 5 years ago by iwakeh

Attachment: metrics-lib.cycle.patch added

comment:8 Changed 5 years ago by karsten

Looks good. I applied and tweaked your patch. Looks like I don't have a public Git repository for metrics-lib, so please find the attached two patches. In particular, see the commit message of 0002 for next steps. Feel free to suggest a better commit message for the squashed commit. Thanks!

Changed 5 years ago by karsten

Changed 5 years ago by karsten

comment:9 in reply to:  8 Changed 5 years ago by iwakeh

I cannot think of a better commit message, but I attach the missing exception, as I didn't
find it in the patch.

Looking forward to java 7!

Changed 5 years ago by iwakeh

comment:10 Changed 5 years ago by karsten

Added the missing exception and rewrote the commit message to:

Remove circular dependencies between packages.

 - Use properties to build impl classes in DescriptorSourceFactory.
 - Move DescriptorParseException out of impl package.  For extra backward
   compatibility, keep a copy of that class in impl, but deprecate it.
 - Upgrade to Java 7, tidy up the build file.

Patch by iwakeh.  Implements #12868.

Will merge into master once we're ready for Java 7.

comment:11 Changed 5 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

I changed my mind about the Java 7 part. I'm having trouble with Java 7 in development and production environment, and this patch shouldn't rely on Java 7. I pushed this branch without the Java 7 specifics, and we can switch to Java 7 later if we want.

Closing, thanks!

Note: See TracTickets for help on using tickets.