[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #30726 [Core Tor/Tor]: Missing relay keys in bandwidth file spec



#30726: Missing relay keys in bandwidth file spec
-------------------------------------+-----------------------------------
 Reporter:  teor                     |          Owner:  (none)
     Type:  defect                   |         Status:  needs_revision
 Priority:  High                     |      Milestone:  sbws: 1.1.x-final
Component:  Core Tor/Tor             |        Version:
 Severity:  Major                    |     Resolution:
 Keywords:  sbws-spec, sbws-roadmap  |  Actual Points:
Parent ID:  #33121                   |         Points:  6
 Reviewer:  ahf, gk                  |        Sponsor:
-------------------------------------+-----------------------------------
Changes (by gk):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:11 juga]:
 > Replying to [comment:10 gk]:
 >
 > > > So i should check the collector to be sure about the which keys were
 in which versions
 > >
 > > Sounds good.
 >
 > I checked and all the collected bandwidth files started with the
 specification version 1.4.0, so in versions earlier than that it does not
 really matter the KeyValues are not correct.
 >
 > Still, i fixed them in their respective versions, including the version
 1.3.0 that was never generated by sbws (it should have been other ticket,
 but the change is small).
 >
 > https://github.com/torproject/torspec/pull/110

 Looks mostly good to me. I have some comments/nits mainly for
 `0b6e97ca37b60fbfbd628306ec4b0551b34142cc`:

 `4ec93f2d8b2add1da598d927785ed7d8e60f0ec3`: looks good

 `0b6e97ca37b60fbfbd628306ec4b0551b34142cc`:

 * `desc_bw_bur`, `consensus_bandwidth`, and
 `consensus_bandwidth_is_unmeasured` are all in `v1.0.3` released which is
 using `SPEC_VERSION` 1.2.0. Thus, those `KeyValues` are already in that
 spec version available
 * in the commit message s/They appeared in in/They appeared in/
 * s/the consensus bandwdith/the consensus bandwidth/
 * In the new "in the last data_period days" parts the default value for
 "data_period days" is not mentioned anymore as it is in previous
 `KeyValue` explanations. Maybe add a hint to the default one, here too
 * In `relay_recent_priority_list_count` there is no "in the last
 data_period days" but there should be, shouldn't it (at least that's what
 `v3bwfile.py` is indicating)?
 * "Add more KeyValues": I think it might make sense to use "Adds more
 KeyValues" in the sense that a particular spec version *adds* those
 things. I am a bit confused though, as the explanations to previous spec
 versions alternate between "Add" and "Adds" without some pattern. You
 could fix that part up in a follow-up commit if you think we should do
 that.

 `fb36a2bae2bc0aa1f0dfbfa68fbb903a5ee6d193`: looks good (modulo the
 previously mentioned "Adds"/"Add" inconsistency; could be part of a
 follow-up fix as well)

 > What i should still check is whether we need to open tickets to metrics
 and stem because of the change of version in the spec. I don't think so
 because there were no bandwidth files published earlier than 1.4.0.

 I agree that it should not be needed from my understanding but, yes, could
 be worth double-checking, though.

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