Re: [tor-bugs] #16596 [Metrics/ExoneraTor]: Change database queries towards making only a single query per request

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#16596: Change database queries towards making only a single query per request
--------------------------------+------------------------------
 Reporter:  karsten             |          Owner:  karsten
     Type:  enhancement         |         Status:  needs_review
 Priority:  Medium              |      Milestone:
Component:  Metrics/ExoneraTor  |        Version:
 Severity:  Normal              |     Resolution:
 Keywords:                      |  Actual Points:
Parent ID:                      |         Points:
 Reviewer:                      |        Sponsor:
--------------------------------+------------------------------
Changes (by karsten):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:8 karsten]:
 > Replying to [comment:6 iwakeh]:
 >
 > Thanks for starting this review!

 Please find
 [https://gitweb.torproject.org/user/karsten/exonerator.git/log/?h=task-16596
 my updated task-16596 branch] with 3 squash commits and 2 additional
 commits.

 > > Already found a bit.  There are also a few things that are part of
 other tickets (#19623, #19624, #21145), which I tried to omit here.
 >
 > Okay.
 >
 > > ExoneraTorServlet: `exoneratorHost` should not be configured via
 web.xml, rather use simple java properties file or even simpler hard code.
 After all it shouldn't change that often, should it?
 >
 > Hmm, right. My idea was that we'll later copy over this servlet to
 metrics-web where it would make a little more sense to make the host name
 configurable. But on second thought that's not really the case. I'll just
 hard-code it.

 Fixed.

 > > In 'step 3' I see some problems with `null` values, for example,
 `"".equals(null)` would evaluate to false (line 147 following).
 >
 > Note that we're using `null` for invalid parameter values and `""` for
 empty parameter values. Do you see actual problems or potential problems
 there?
 >
 > In the latter case (potential problems), maybe we can resolve them by
 documenting things a bit better, or by making things clearer in subsequent
 commits.
 >
 > In the former case (actual problems), let's of course fix the issues
 now. But I'd have to see the actual problem first.
 >
 > > For most exceptions caught the error message should be logged; and, it
 might be time to switch to slf4j-api and logback, now.
 >
 > Yes, we can switch to slf4j, though we should do that in a separate
 commit on top of these, probably in a new ticket.
 >
 > > In addition, reading the response query should also catch
 RuntimeExceptions (possibly from Gson).
 >
 > Ugh, indeed. Will fix. Good '''catch'''. :)

 Fixed.

 > > The version received should also be logged, if it differs from the
 known version.
 >
 > Yep, good idea.
 >
 > > The known version(s) could be a constant in `ExoneraTorServlet`;
 either just the major part or all.  This makes it more obvious.
 >
 > Also a good idea.

 Both fixed.

 > > QueryServlet: Helper methods ` private String parseTimestampParameter`
 and `private String parseIpParameter` should be `public static` and tests
 should be added.  Similarly, `private String convertIpV*ToHex`, which
 could be combined by simply adding another argument, i.e., `public static
 String convertIpV4ToHex(String relayIp, boolean isIpV4)`.
 >
 > Agreed, though let's save that for another ticket and not overload this
 ticket with too many improvements all at once. I know it's tempting. Stay
 strong, we'll eventually fix these things.
 >
 > > A `MILLISEC_IN_DAY = 24L * 60L * 60L * 1000L` constant would be
 useful.

 Fixed.

 > Agreed. That's probably simple enough for a separate commit on top of
 this branch.
 >
 > > Other: Using the object `Boolean exit` for a triplet state is a bit
 too much of a hack.  There could be a simple enum, as simple as, for
 example, `public enum Exit { U, Y, N; }`.
 >
 > Ugh, that would turn the JSON field into a string, which doesn't seem
 very intuitive for "is an exit". And whoever processes this JSON will need
 to check for `null` anyway, regardless of boolean or string. In fact, I
 think that we're using the boolean field exactly in the way it's supposed
 to be used: `true` means "is an exit", `false` means "is not an exit", and
 `null` means "we don't know". I think I'd like to leave this one
 unchanged. It's not a hack.
 >
 > > Regarding SQL:
 > > Order-by statement should be using names not numbers.
 >
 > Hmm, now that you mention that, I wonder if we even need results to be
 sorted at all. I'll try to take that out. Otherwise I'd change numbers to
 names in a subsequent commit, because it's not directly related to this
 ticket.

 Fixed.

 > > Couldn't the exit-information be part of the query already?
 >
 > Yes, but I'd like to save database changes for a later ticket. There's
 so, so much more we can do to make the database schema more efficient.
 (I'd have to look at my notes from last year, but I believe that we're
 storing exit information directly in the database rather than the entire
 raw status entry.)
 >
 > > (I could also look into providing some patches, if we agree on the
 changes and you don't have the time?)
 >
 > I'll try to make changes tomorrow morning. Thanks again for the initial
 review!

 Want to take another look at the revised branch? Anything else that needs
 changing before this can go into master (after squashing the squash
 commits)? Thanks!

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs