Opened 3 years ago

Closed 3 years ago

#19776 closed enhancement (fixed)

Make minor improvements to scheduler

Reported by: karsten Owned by: iwakeh
Priority: High Milestone: CollecTor 1.0.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

While running some local performance tests of the bridge descriptor sanitizer I noticed a few possible improvements:

  • I noticed that the default configuration changed some paths, like, move multiple directories under out/. Can we undo that change, so that CollecTor uses the exact same paths in the new default configuration as previously?
  • The recent/ directory is not truly configurable. I can change that along with the other bugfixes to the bridge descriptor sanitizer, if you want.
  • It seems like the scheduler calculates how many minutes it needs to sleep from the current system time to start the next run with x minutes offset from the period of y minutes. Example: system time is 20:43:15.123, period is 60 minutes, offset is 44 minutes, the scheduler will start the next run at 20:44:15.123, but really it should start it at 20:44:00.000.
  • Can we add a mode to run a given module immediately, skipping the scheduler entirely? That would be quite useful for testing, batch-processing, and maybe other purposes. Onionoo has such a mode with --single-run (or however it's called). But please don't break the design too much to make this work, if it's too ugly, skip this.

Child Tickets

Change History (10)

comment:1 in reply to:  description Changed 3 years ago by iwakeh

Owner: set to iwakeh
Status: newassigned

Replying to karsten:

While running some local performance tests of the bridge descriptor sanitizer I noticed a few possible improvements:

  • I noticed that the default configuration changed some paths, like, move multiple directories under out/. Can we undo that change, so that CollecTor uses the exact same paths in the new default configuration as previously?

Well, I thought because of comment6 these were already corrected to the paths use in the main CollecTor instance.
Please adjust these paths with the sanitizer ticket. My CollecTor instance was configured differently from the beginning and I would have to guess everything.

  • The recent/ directory is not truly configurable. I can change that along with the other bugfixes to the bridge descriptor sanitizer, if you want.

Planned this for the upcoming implementation of #19720. But it might better fit into the sanitizer fixes.

  • It seems like the scheduler calculates how many minutes it needs to sleep from the current system time to start the next run with x minutes offset from the period of y minutes. Example: system time is 20:43:15.123, period is 60 minutes, offset is 44 minutes, the scheduler will start the next run at 20:44:15.123, but really it should start it at 20:44:00.000.

A feature not a bug :-) As the time resolution is minutes the start second shouldn't matter.
But it makes sense to change this the way you suggested. I'll look into it.

  • Can we add a mode to run a given module immediately, skipping the scheduler entirely? That would be quite useful for testing, batch-processing, and maybe other purposes. Onionoo has such a mode with --single-run (or however it's called). But please don't break the design too much to make this work, if it's too ugly, skip this.

I'll take a look at this, too.

comment:2 Changed 3 years ago by iwakeh

Status: assignedneeds_review

Please review my branch.
It contains the patch for 3, the HH.mm:00 part.
The milliseconds are not easily set to zero, b/c the scheduler seems to take time to schedule a task.
When the period is already running the start happens immediately, i.e. 10:10:03 with period 2 and offset 0 the task will be started at 10:10:03.
I didn't find a better solution here, will this suffice?

Regarding part 4:
It fits into the current design, but I rather have new properties instead of a command line switch. That way all settings are in one file, which helps with (remote) debugging. Will that do?

comment:3 Changed 3 years ago by iwakeh

part 1: solved in #19840.
part 2: see #19720.

comment:4 Changed 3 years ago by iwakeh

Priority: MediumHigh

Increasing priority, so this floats near the top of the stack of reviews.

comment:5 Changed 3 years ago by karsten

I tried to tweak your branch by converting everything to milliseconds. Please take a look at my branch task-19776. Some initial tests looked promising, but I'm not sure if I overlooked something. Please review carefully!

Your suggestion for part 4 sounds reasonable.

comment:6 in reply to:  5 Changed 3 years ago by iwakeh

Replying to karsten:

I tried to tweak your branch by converting everything to milliseconds. Please take a look at my branch task-19776. Some initial tests looked promising, but I'm not sure if I overlooked something. Please review carefully!

Fine to have a more elaborate log message there. I made the calculation testable and included a simple test. Feel free to add some more values. This branch is based on yours.

Your suggestion for part 4 sounds reasonable.

Ok, I'll get to this next.

comment:7 Changed 3 years ago by iwakeh

Added a patch for part4-single-run including a test; adapted coverage percentages.

Please review the change.

comment:8 Changed 3 years ago by karsten

Alright, I made some trivial changes (avoid mixing minutes and milliseconds in computeInitialDelayMillis() and change unit tests to make period greater than offset) and some non-trivial rebasing and finally came up with what you can find in my task-19776-2 branch. That branch contains fixes for parts 3 and 4 discussed here. Want to take a final look before I push to master?

comment:9 Changed 3 years ago by iwakeh

Looks fine and thanks for unifying the time units :-)

comment:10 Changed 3 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Cool! Pushed to master, closing. Thanks for making these improvements!

Note: See TracTickets for help on using tickets.