Opened 6 months ago

Closed 5 months ago

#25760 closed defect (implemented)

Remove TestingEnableTbEmptyEvent if it is no longer used

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 034-roadmap-subtask
Cc: karsten, robgjansen Actual Points:
Parent ID: #25373 Points:
Reviewer: dgoulet Sponsor: Sponsor8-can

Description

The TestingEnableTbEmptyEvent logic uses a lot of code and complicates our token bucket refill logic. If we need to keep it working, we can -- but it would be much simpler to remove it, if it is not in use.

Is anybody still using it? We seem to have added it in 0.2.5.2-alpha, back in 2013.

Child Tickets

Change History (8)

comment:1 Changed 6 months ago by robgjansen

We are collecting and logging these events in our Shadow experiments, but we haven't recently been using the data in any analysis. At this point, I think the benefits of cleaner code outweighs the benefits of keeping the event around. Thank you for checking!

comment:2 Changed 6 months ago by nickm

Thanks, Rob!

Is there anybody else you think I should ask? Otherwise I think I should remove these fast, since they're getting in the way.

comment:3 Changed 6 months ago by nickm

Status: assignedaccepted

In the meantime

comment:4 Changed 6 months ago by nickm

Status: acceptedneeds_review

whoops, hit "submit changes" too early.

In the meantime, could I have a review on my branch remove_tb_empty?

comment:5 Changed 6 months ago by nickm

Reviewer: dgoulet

comment:6 in reply to:  2 Changed 6 months ago by robgjansen

Replying to nickm:

Thanks, Rob!

Is there anybody else you think I should ask? Otherwise I think I should remove these fast, since they're getting in the way.

Since this is a testing network event, I think the primary consumers would be Shadow and chutney. For Shadow, there is a chance that some researcher out there is using the event, but I doubt this is the case (and I don't recalling having been asked any questions about it). For chutney, I think you and teor are among the main users. The only other person that comes to mind is Karsten.

comment:7 Changed 5 months ago by nickm

Okay, we've run out of time on merging the parent, so we're going to merge this removal and hope for the best. If it turns out we do need this feature, we'll have to restore it and refactor it to use the new token buffer code.

comment:8 Changed 5 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.