Opened 5 months ago

Closed 4 months 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

Description

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 5 months ago by nickm

  • Owner set to nickm
  • Status changed from new to accepted

comment:2 Changed 4 months ago by dgoulet

  • Keywords prop140 added

comment:3 Changed 4 months ago by nickm

  • Actual Points set to 1
  • Status changed from accepted to needs_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 months ago by nickm

  • Keywords review-group-17 added

comment:5 Changed 4 months ago by dgoulet

  • Reviewer set to ahf

comment:7 Changed 4 months ago by ahf

  • Status changed from needs_review to needs_revision

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

comment:8 Changed 4 months ago by nickm

  • Status changed from needs_revision to needs_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 months ago by asn

  • Status changed from needs_review to needs_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 months ago by nickm

  • Status changed from needs_revision to needs_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 months ago by asn

  • Status changed from needs_review to merge_ready

All fixes look good to me!

I'll turn this to merge_ready.

comment:12 Changed 4 months ago by nickm_mobile

  • Actual Points changed from 1 to 2
  • Resolution set to implemented
  • Status changed from merge_ready to closed

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

Note: See TracTickets for help on using tickets.