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_information => needs_review


Comment:

 Thanks for the review. However, I think we'll need to resolve two open
 discussion points before moving forward here:

  1. I still believe that `exit` should be a boolean value rather than a
 one-letter string. This is quite clearly a `true`/`false` question, with
 `null` being the exception case where we don't know. I think it's valid to
 define the protocol with a field that may be omitted if unknown. We're
 doing that in Onionoo, too, and that's not a bug. We wouldn't, for
 example, change Onionoo's `hibernating` or `recommended_version` to
 `string` with `Y`/`N`/`U` only to reflect that the field may contain
 something else than `true` or `false`. Even worse, with `Y`/`N`/`U` as
 string values, an implementation would still have to check for `null` and
 handle that case, or risk running into a `NullPointerException`. In short,
 I don't see a reason for switching the `Boolean` there to `String` or
 `Enum`. We shouldn't start doing this in ExoneraTor, and we shouldn't do
 it elsewhere.

  2. Let's not move around any more code than necessary at this point. As I
 wrote in one of the commit messages: ''"Note that some code duplication
 between ExoneraTorServlet and QueryServlet was deemed acceptable, because
 ExoneraTorServlet will be moved to metrics-web in the medium term
 anyway."'' I understand the desire to create an `ExoneraTorUtils` class
 for methods that are otherwise duplicated. But I'd like us to move
 `ExoneraTorServlet` away to metrics-web soon after this branch is merged,
 and in that case a utils class wouldn't make much sense anymore.
 Similarly, let's hold back adding new unit tests until `ExoneraTorServlet`
 is gone. This branch is a work in progress, not the shiny new ExoneraTor
 that we'll keep unchanged for the next five years to come.

 If you agree, I'm happy to cherry-pick the changes from your branch that I
 think can go in at this point. There are certainly a lot of changes that
 I'd like to keep. But if you disagree, I'd like to first discuss those two
 points before moving forward. What do you say?

 (Setting to '''needs_review''' to indicate that this ticket needs input
 from the reviewer, not because there's new code to review.)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16596#comment:12>
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