Opened 5 years ago

Closed 5 years ago

#12866 closed defect (fixed)

cycle between util and docs packages

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

Description

there is some cycling between docs and util:

DateTimeHelper and ApplicationFactory from util depend on the docs package.
For ApplicationFactory merely the get/setDocumentStore need to be moved,
i.e. to a docs.DocumentStoreFactory class.

DateTimeHelper is heavily used in several docs classes and
also in util.Logger. It would be nice to have the DateTimeHelper in docs.
But Logger only uses the ONE_MINUTE and ONE_SECOND constants from
DateTimeHelper. Thus, things could be easily resolved.

Child Tickets

Change History (10)

comment:1 Changed 5 years ago by karsten

Agreed that circular dependencies are a bad idea. I split up the ApplicationFactory class into four factory classes. Do you mind reviewing the changes? https://gitweb.torproject.org/user/karsten/onionoo.git/shortlog/refs/heads/task-12866

However, I'm not convinced that DateTimeHelper should move to the docs package. It's a utility class used by many other classes in the other packages. Moving it to the docs package seems arbitrary. Thoughts?

FWIW, the dependency between Logger and DateTimeHelper could be removed by simply defining ONE_SECOND and ONE_MINUTE in Logger.

comment:2 Changed 5 years ago by iwakeh

the four factory approach looks good.

Maybe util.Time would be a good place for all the time constants?
But if DateTimeHelper stays in util, this move might not be necessary either.
So the DateTimeHelper relocation might need more thought. I intended
to make the protocol api separation easier ...

comment:3 Changed 5 years ago by karsten

Okay, testing the four factory thing locally, will probably merge tomorrow, unless something breaks horribly.

I'll also think more about moving DateTimeHelper to docs. Maybe we should internally work with timestamps in milliseconds and only format/parse timestamps to Strings when writing/reading docs. That would mean, for example, that GraphHistory.setFirst() expects a long and uses DateTimeHelper to format to set its String attribute. And in the very few other cases where we have to format/parse timestamps we can just use SimpleDateFormat directly. The main reason for this helper class was to avoid instantiating tons of SimpleDateFormats. 99% of those would have happened in docs classes.

comment:4 Changed 5 years ago by iwakeh

I think the docs package is the right place for DateTimeHelper and it should not be
torn apart b/c

  • most usages of parsing/formatting occur there and
  • all other classes needing that functionality depend on the docs package anyway and even more important only use DateTimeHelper in order to create Documents (afaik).
  • Keeping the entire formatting/parsing in on place is easier to test and maintain. (the future (i.e. java8) provides a new package java.time for handling temporal things.)

comment:5 Changed 5 years ago by karsten

Merged the four factory change.

I think I agree with your reasons for moving DateTimeHelper to docs. Though I'm still looking into ways that don't involve adding new dependencies between packages. For example, a writer class doesn't have to format a timestamp only to pass it to a docs class; it can as well pass the timestamp in milliseconds and expect the docs class to do the right thing. I'm working on this right now. If you want, feel free to review this branch: https://gitweb.torproject.org/user/karsten/onionoo.git/shortlog/refs/heads/task-12866-2

comment:6 in reply to:  5 Changed 5 years ago by iwakeh


... For example, a writer class doesn't have to format a timestamp only to pass it to a docs class; it can as well pass the timestamp in milliseconds and expect the docs class to do the right thing.

That sounds like a good solution to me, too.

I'm working on this right now. If you want, feel free to review this branch: https://gitweb.torproject.org/user/karsten/onionoo.git/shortlog/refs/heads/task-12866-2

I'll take a look at it.

comment:7 Changed 5 years ago by karsten

Status: newneeds_review

Just pushed more commits to that branch, including a commit that moves the DateTimeHelper class to docs. Needs review and testing. If you can take a look, awesome! If not, I'll do that later today and tomorrow.

comment:8 Changed 5 years ago by iwakeh

great, thanks for the move!

The multiplication of time constants should be OK, as the number of millisecs per day, min, etc. doesn't change that often ;-)

some minor issue, but maybe i overlook something obvious:
Why not make SimpleDateFormat a static field in classes like NodeIndex?
This wouldn't cause concurrency issues, afaik.

I didn't have time yet to compile anything nor test.

I added some thoughts about making docs independent from util here #12869#comment:3

Last edited 5 years ago by iwakeh (previous) (diff)

comment:9 Changed 5 years ago by karsten

Agreed that SimpleDateFormat could be made a static field. I'm just careful, because I've been bitten by that class not being thread-safe. That shouldn't be a problem here. However, whether we're instantiating two SimpleDateFormats every hour or re-using a static one doesn't make much of a difference.

Will do another review, run some tests, and then merge and deploy. That's going to take a while though. (Will also reply to #12869 later on that ticket.)

comment:10 Changed 5 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Reviewed, tested, merged, deployed. Looks good! Thanks for the suggestion and discussion here!

Note: See TracTickets for help on using tickets.