[metrics-bugs] #31558 [Metrics/CollecTor]: Process bridge pool assignments again

Tor Bug Tracker & Wiki blackhole at torproject.org
Wed Sep 11 13:22:50 UTC 2019


#31558: Process bridge pool assignments again
-------------------------------+------------------------------
 Reporter:  karsten            |          Owner:  metrics-team
     Type:  enhancement        |         Status:  needs_review
 Priority:  Medium             |      Milestone:
Component:  Metrics/CollecTor  |        Version:
 Severity:  Normal             |     Resolution:
 Keywords:  BugSmashFund       |  Actual Points:
Parent ID:                     |         Points:
 Reviewer:  irl                |        Sponsor:
-------------------------------+------------------------------
Changes (by karsten):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:9 fava]:
 > Replying to [comment:7 karsten]:
 >
 >
 > > Please review
 [https://gitweb.torproject.org/user/karsten/collector.git/commit/?h=task-31558&id=db2a6bdab1bfc12377c515577589aa67d34fa2ba
 commit db2a6bd in my task-31558 branch].
 >
 > Hi karsten,
 >
 > it is my first review so let me know if I could do better.
 >
 >  1. Please order statement following
 [https://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141855.html#1852
 Java guidelines] . This guidelines was been written in 1999 but is a good
 point to start and it is used also in static code analysis

 We do have guidelines for ordering parts in a class:
 https://trac.torproject.org/projects/tor/wiki/org/teams/MetricsTeam/MetricsJavaStyleGuide

 What we'd really need is a way to automatically check this, just like how
 we have Checkstyle to check code style violations. Would you want to try
 out some tools that can do these checks? Requirement is that we can obtain
 them using `ant resolve` (using Ant Ivy internally) and that we can run
 them from Ant just like we're running Checkstyle. (This would be something
 for a new ticket.)

 >  2. Probably it make sense to create a constant for string
 "BridgePoolAssignments"

 One might think so, but these two strings actually mean something
 different. I'd rather not want to use a single constant for them, because
 that would imply that they're the same thing. What I'd really want is give
 up on using these string constants, but that's a larger refactoring than
 I'd want to do at the moment.

 >  3. Instead of evaluate each time `330L * 60L * 1000L` it is better to
 create a constant with meaningfull name

 This is already changed in the more recent branch.

 >  4. Instead of evaluate each time `3L * 24L * 60L * 60L * 1000L` it is
 better to create a constant with meaningfull name

 I changed this in
 [https://gitweb.torproject.org/user/karsten/collector.git/commit/?h=task-31558&id=9c1acf80a14eec1f54f456f23200a5ff70cd9c0f
 commit 9c1acf8 in my task-31558 branch].

 >  5. startProcessing it seems to complex, is it possible to split in more
 separated and testeable methods?

 I already changed this in the more recent branch, too.

 >  6. there are some for each loop, is it make sense use a functional
 approach using lambda?

 Maybe, but to me this falls into the category of prettying code rather
 than reviewing that it's good to go in. It's more like the try-with-
 resource stuff that we could do anytime but which shouldn't block the
 merge in this case.

 > Please let me know if I could help you in some way,
 >
 > Best Regards

 Thanks for doing this review!

 Here's one suggestion for the next review: Be sure to not only review the
 first commit, but also consider any follow-up commits in that branch. The
 way I'd do that is check out the branch and look at `git diff
 $beforefirstcommit..`, e.g., `git diff 8010084..` in this case.

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


More information about the metrics-bugs mailing list