Opened 4 years ago

Closed 4 years ago

#21651 closed defect (implemented)

prop140/compression: Refactor directory cache spooling code

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201703, prop140, review-group-17
Cc: Actual Points: 2
Parent ID: Points: 3
Reviewer: ahf Sponsor: Sponsor4


Our current logic for spooling things from a directory server is a bit loopy. Instead we should refactor it so that "things to be spooled" is a first-class object, with implementations depending on what we're spooling. This will make lots of our directory improvement stuff easier to implement.

Child Tickets

Change History (12)

comment:1 Changed 4 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 4 years ago by dgoulet

Keywords: prop140 added

comment:3 Changed 4 years ago by nickm

Actual Points: 1
Status: acceptedneeds_review

See branch spooling in my public repository. The code here is already covered by the unit tests combined with the chutney integration tests, but please let me know if you think it could use more testing.

comment:4 Changed 4 years ago by nickm

Keywords: review-group-17 added

comment:5 Changed 4 years ago by dgoulet

Reviewer: ahf

comment:7 Changed 4 years ago by ahf

Status: needs_reviewneeds_revision

Minor questions on GL. Could use one additional set of eyes.

comment:8 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

I've responded to the questions, added a typo fix, and moved this back into needs_review for the additional set of eyes.

comment:9 Changed 4 years ago by asn

Status: needs_reviewneeds_revision

Hello, I did a review for this ticket on gitlab. Please check it out! :)

Also, I must say that the commit structure was not very helpful to review, especially since I'm not familiar with that part of the codebase (dirserv/connection logic):

8587f66 was pretty small and nice, but 6316f23 did everything and the kitchen sink. It moved code, it removed code and it added new code without splitting it into separate commits. So I had to continuously check between the code and the diff to see whether what I'm reviewing is new or old and how it used to be. I think if the branch was split into 3-4 commits it would be easier to review (e.g. one commit introduces the spooling structures and basic API, another commit moves old code to new functions, another commit edits such code, another commit introduces new code).

Hope the review is useful!

comment:10 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

Thanks, asn. I'll try to split things up better in the future. Please remind me if I don't, or just ask me to revise.

I've tried to fix the issues you found on the gitlab review, and added a couple of questions there about whether you think particular changes are needed. Putting this back in needs_review.

comment:11 Changed 4 years ago by asn

Status: needs_reviewmerge_ready

All fixes look good to me!

I'll turn this to merge_ready.

comment:12 Changed 4 years ago by nickm_mobile

Actual Points: 12
Resolution: implemented
Status: merge_readyclosed

Squashed, merged, pushed. Thanks for reviewing, ahf and asn!

Note: See TracTickets for help on using tickets.