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:
--------------------------------+------------------------------

Comment (by iwakeh):

 Replying to [comment:12 karsten]:
 > 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.

 Hmm ..., ok.  Let 'exit' stay Boolean.

 >
 >  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.

 I can do without the ExoneraTorUtils.  Still the methods that only return
 something based on the input they received should become 'static', which
 will make it easier to integrate this code later in metrics-web (and also
 find out there what duplication there is).

 Anyway, even work-in-progress should be readable, without redundancies,
 and well encapsulated.  Thus, I would want to keep the Gson/JSON
 encapsulation and the tests for `QueryResponse`.  These are `living`
 documentation of the protocol, which we don't want to write a spec for
 (yet).  There has to be a way to determine what was intended to be
 programmed other than by looking at the running code.  How to tell
 undiscovered bugs apart from features?  (And, all what was agreed in
 tickets doesn't count as documentation, i.e., maybe document the thought
 about the move of `ExoneraTorServlet` in the class and explain the current
 state of code?)

 >
 > 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?

 I assume you would want to keep logging streamlining as well as the
 QueryResponse tests.
 (I noticed that `QueryServlet` is still using the jul; I'll provide a
 fixup commit.)

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