Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#28446 closed defect (fixed)

Change sbws scaling method to torflow

Reported by: teor Owned by:
Priority: Medium Milestone: sbws: 1.0.x-final
Component: Core Tor/sbws Version:
Severity: Normal Keywords: sbws-1.0-must-closed-moved-20181128
Cc: pastly, juga Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:

Description

I tried to fix this as part of #28442, but I couldn't get it to work:

# TODO: change scaling_method to TORFLOW_SCALING before getting this
# in production

https://github.com/torproject/sbws/blob/821f86e8392bc05579f7785e6907be6cc3844fef/sbws/lib/v3bwfile.py#L484

Are there any other TODOs that should be fixed?

Child Tickets

Change History (10)

comment:1 Changed 12 months ago by juga

That was fixed in
https://gitweb.torproject.org/sbws.git/commit/?id=835255703d582964a0e564d81a7848e87c5568cd, i forgot to remove the todo.

I agree it's also confusing that if pass that option, it does the opposite, so i should change the name and/or add help text.

comment:2 Changed 12 months ago by teor

I don't think it is completely fixed, because the default is still None in some places:
https://github.com/torproject/sbws/blob/master/sbws/lib/v3bwfile.py#L461

comment:3 in reply to:  1 Changed 12 months ago by juga

Replying to teor:

I don't think it is completely fixed, because the default is still None in some places:
https://github.com/torproject/sbws/blob/master/sbws/lib/v3bwfile.py#L461

The default was None there, but it's being called by generate, which has torflow's method as default one.

Replying to juga:

That was fixed in
https://gitweb.torproject.org/sbws.git/commit/?id=835255703d582964a0e564d81a7848e87c5568cd, i forgot to remove the todo.

I agree it's also confusing that if pass that option, it does the opposite, so i should change the name and/or add help text.

I've done this in https://github.com/juga0/sbws/tree/ticket28446.

I'm not changing this ticket to needs_review. If you're going to review it, let me know assigning yourself as reviewer, otherwise i'll merge and close in one week.

comment:4 Changed 12 months ago by juga

Note for irc #tor-bots humans, trac-sbws cced people and myself: Yes, Ci failed, i need to review it. Please don't consider this as something to review while i don't notify i've checked and/or fixed Ci.

comment:5 in reply to:  4 Changed 12 months ago by teor

Reviewer: teor
Status: newneeds_revision

Replying to juga:

Note for irc #tor-bots humans, trac-sbws cced people and myself: Yes, Ci failed, i need to review it. Please don't consider this as something to review while i don't notify i've checked and/or fixed Ci.

CI fails because the unit tests assume that sbws scaling is the default.
If you don't want people to review a ticket, use the needs_revision status.

I tried to review this ticket, but there is no pull request, so I didn't know where to put my comments.

Here is my review:

  • If you remove an option, anyone who uses the option will get an error. So you should either:
    • accept the option, but ignore it, or
    • increment the sbws version to 2.0

comment:6 Changed 12 months ago by juga

Status: needs_revisionneeds_review

comment:7 Changed 12 months ago by teor

Status: needs_reviewneeds_revision

I added two questions on the pull request.

Feel free to make changes, or merge as-is.

comment:8 Changed 12 months ago by juga

Status: needs_revisionmerge_ready

ok, modified. I'll squash and reword commits lines, since now they won't match.

comment:9 Changed 12 months ago by juga

Resolution: fixed
Status: merge_readyclosed

comment:10 Changed 12 months ago by teor

Keywords: sbws-1.0-must-closed-moved-20181128 added
Milestone: sbws 1.0 (MVP must)sbws: 1.0.x-final

Move all closed sbws 1.0 must tickets to sbws 1.0.x-final

Note: See TracTickets for help on using tickets.