Opened 3 years ago

Closed 3 years ago

#21443 closed defect (fixed)

CollecTor does not delete exit lists after three days anymore

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone: CollecTor 1.2.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Today I learned by a coincidence that CollecTor does not keep its directory with recent exit lists clean anymore. Here's the bug:

diff --git a/src/main/java/org/torproject/collector/exitlists/ExitListDownloader.java b/src/main/java/org/torproject/collector/exitlists/ExitListDownloader.java
index 877057e..0ac5112 100644
--- a/src/main/java/org/torproject/collector/exitlists/ExitListDownloader.java
+++ b/src/main/java/org/torproject/collector/exitlists/ExitListDownloader.java
@@ -199,7 +199,7 @@ public class ExitListDownloader extends CollecTorMain {
     long cutOffMillis = System.currentTimeMillis()
         - 3L * 24L * 60L * 60L * 1000L;
     Stack<File> allFiles = new Stack<File>();
-    allFiles.add(new File(recentPathName, EXITLISTS));
+    allFiles.add(new File(recentPathName));
     while (!allFiles.isEmpty()) {
       File file = allFiles.pop();
       if (file.isDirectory()) {

This doesn't require a patch release immediately, but having a release later this month would be good.

Child Tickets

Change History (6)

comment:1 Changed 3 years ago by karsten

Status: newneeds_review

comment:2 Changed 3 years ago by iwakeh

Hmm, wouldn't that clean the entire recent-dir?

And, it would be nice to have a test for this. It would be a good preparation for introducing CleanUtils (#20546 which I have on my list review and some tweaking).

comment:3 in reply to:  2 Changed 3 years ago by iwakeh

Replying to iwakeh:

Hmm, wouldn't that clean the entire recent-dir?

Retracting this question, line 72 explains. Looks fine.

And, it would be nice to have a test for this. It would be a good preparation for introducing CleanUtils (#20546 which I have on my list review and some tweaking).

Still applies ;-)

And, maybe some modernization regarding download and persisting the lists?
But, that extends the scope of this ticket.

comment:4 Changed 3 years ago by karsten

Hmm, can you come up with a simple test for this bugfix?

If not, I'd argue that fixes of recently introduced bugs don't need to be accompanied by tests, as opposed to enhancements. Which probably means that we should have written a test when changing this code earlier, which might have uncovered the bug in the first place. But really, I can see this trivial fix go in without test.

And yes, modernization of this code is beyond the scope of this ticket, unfortunately. We should probably think about that when adding other data to CollecTor, like webstats. But that's for another day.

comment:5 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.2.0
Status: needs_reviewmerge_ready

Making the code testable is part of the modernization. And, you're right this is part of another ticket, which already exists: #20516
Ticket #20516 and the related ones are currently part of the next milestone 1.2.0.

I added this ticket to milestone 1.2.0.

Merge ready.

comment:6 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

Great, pushed to master. Closing. Thanks!

Note: See TracTickets for help on using tickets.