actively monitor resources like available storage space

changed milestone to %CollecTor 1.0.0

added component::metrics/collector ctip milestone::CollecTor 1.0.0 owner::iwakeh priority::medium resolution::fixed severity::normal status::closed type::enhancement labels

I think I'd rather want us to fix the underlying problem of using massive amounts of disk space. The best way to achieve that is to improve how we're producing (xz-compressed) tarballs. Right now, we're creating those tarballs by first writing the uncompressed tarball to disk and then compressing it. Oh, and of course, we have the directories on disk that we're creating tarballs from. I hope we can find a better way. (If there's already a ticket for that, please feel free to paste a link to this ticket.)

I also just asked the Tor admins to send me automatic email notifications in case the disk is about to run full. That should be a good enough band-aid.

Sound okay?

Replying to karsten:

I think I'd rather want us to fix the underlying problem of using massive amounts of disk space. The best way to achieve that is to improve how we're producing (xz-compressed) tarballs. Right now, we're creating those tarballs by first writing the uncompressed tarball to disk and then compressing it. Oh, and of course, we have the directories on disk that we're creating tarballs from. I hope we can find a better way. (If there's already a ticket for that, please feel free to paste a link to this ticket.)

Yes, this is fine. Let's use the above list for future discussion here.

I also just asked the Tor admins to send me automatic email notifications in case the disk is about to run full. That should be a good enough band-aid.

Sound okay? That's fine, too. (I actually thought that this was the case already ;-)

Would it make sense to have the "is there enough space check?" immediately at the beginning of each run? That way in case of a shortage only things like votes and consensus etc. could be retrieved, and more space consuming tasks could be postponed. Thus, avoiding the overfull disk?

I'd say let's add a warning if there's not much space left on the device at the beginning of a run (less than 200M?). But let's not make CollecTor behave differently when it finds that it's low on disk space. If we have such warnings in place (the Nagios notification and the new low-on-disk-space warning), we should assume that operators are smart enough to do the right thing. The recent problems only happened, because we don't have any notifications at all. But acting differently in case of low disk space would be like a car on reserve that doesn't let you drive faster than 100 km/h or not in opposite direction of a nearby gas station. Oh, I would hate a car that thinks it's smarter than I, even if it is.

Yes, agreed; humans decide not algorithms ;-)

todo: log warnings in case of insufficient space (start with 200M limit) It might be good to also check at the end of a run, as there would be more time for operators to react.

Trac:
Status: new to accepted

Added to first release milestone.

Trac:
Milestone: N/A to CollecTor 1.0.0

Please review this branch.

Currently, the check is only added to ArchiveWriter. Where else should it go?

I didn't make the limit configurable, as the properties file is really big already and the space needed depends on the download and will be the same for all instances. But, if there is a good reason for adding it to the configuration that's not a big deal.

Trac:
Status: accepted to needs_review

Change looks good, just minor nitpicks, and I'm happy to make all changes if you're okay with that:

Some lines exceed the implicit 74 character limit that I have been using in the past and that we might not have included in the styleguides yet. Let me explain: 74 characters is the maximum number of characters that look reasonable in vim with line numbers turned on in files up to 999 lines. Somewhat arbitrary, I know. Are there arguments in favor of other line lengths? If so, let's discuss those. Otherwise, let's just keep 74.
Can we remove those newlines after closing brackets?
I wonder how the Javadoc with three sentences without newline between first and second and without

around second and third sentence conforms with checkstyle. Can we rephrase those three sentences to a single sentence or separate them into one sentence and a paragraph?
Both 1024. and Math.floor seem unnecessary for long values. Can we change them to 1024 and omit the flooring?
I think the preferred Git message format is: "summary of no more than 50 chars, newline, one or more parapraphs with no more than 70 chars per line". (Note: I sometimes exceed the 50, and I'm open to using something else than 70, for example, 72 which I just read on a random blog post.) Do you mind if we change the message to something like the following?

Check available disk space in relaydescs module.

Check if there's enough space before and after running the relaydescs
module, and warn if less than 200 MiB are left.

Implements #18865.

So, mostly whitespace complaints. :) But I figured it's better to bring them up at some point to make future reviews faster. Again, happy to change things if you agree.

Trac:
Status: needs_review to needs_revision

Thanks for reviewing so quickly!

Some lines exceed the implicit 74 character limit that I have been using in the past and that we might not have included in the styleguides yet. Let me explain: 74 characters is the maximum number of characters that look reasonable in vim with line numbers turned on in files up to 999 lines. Somewhat arbitrary, I know. Are there arguments in favor of other line lengths? If so, let's discuss those. Otherwise, let's just keep 74.

Well, we actually have a None..None(https://trac.torproject.org/projects/tor/wiki/org/teams/MetricsTeam/MetricsJavaStyleGuide#s4.4-column-limit) column limit, but I prefer to keep it between 70 and 80. That's why this shows up repeatedly.

Please, make the changes to the guide doc, but I prefer 80.

(I vaguely remember that we agreed on 76-80 in one meeting, but I can't find the minutes.)

74 is too short and the problem will become worse with java8 and using lambda expressions and more fluent style. The coding style changed and we all use screens way bigger than in the last century. A line of length 80 can easily be read with one glance still.

Can we remove those newlines after closing brackets?

I wonder how the Javadoc with three sentences without newline between first and second and without

around second and third sentence conforms with checkstyle. Can we rephrase those three sentences to a single sentence or separate them into one sentence and a paragraph?

I usually run ant checks before committing and it doesn't complain. The three sentence javadoc will become one paragraph, but when editing it later it's easier to see, which sentence was changed.

Both 1024. and Math.floor seem unnecessary for long values. Can we change them to 1024 and omit the flooring?

Oops, please change.

I think the preferred Git message format is: "summary of no more than 50 chars, newline, one or more parapraphs with no more than 70 chars per line". (Note: I sometimes exceed the 50, and I'm open to using something else than 70, for example, 72 which I just read on a random blog post.) Do you mind if we change the message to something like the following?

{{{ Check available disk space in relaydescs module.

Check if there's enough space before and after running the relaydescs module, and warn if less than 200 MiB are left.

Implements #18865 (moved). }}}

That's fine! And the git-msg format should have a place in the docs somewhere.

So, mostly whitespace complaints. :) But I figured it's better to bring them up at some point to make future reviews faster. Again, happy to change things if you agree.

Please do. Thanks a lot!

Replying to iwakeh:

Thanks for reviewing so quickly!

Some lines exceed the implicit 74 character limit that I have been using in the past and that we might not have included in the styleguides yet. Let me explain: 74 characters is the maximum number of characters that look reasonable in vim with line numbers turned on in files up to 999 lines. Somewhat arbitrary, I know. Are there arguments in favor of other line lengths? If so, let's discuss those. Otherwise, let's just keep 74.

Well, we actually have a None..None(https://trac.torproject.org/projects/tor/wiki/org/teams/MetricsTeam/MetricsJavaStyleGuide#s4.4-column-limit) column limit, but I prefer to keep it between 70 and 80. That's why this shows up repeatedly.

Please, make the changes to the guide doc, but I prefer 80.

(I vaguely remember that we agreed on 76-80 in one meeting, but I can't find the minutes.)

74 is too short and the problem will become worse with java8 and using lambda expressions and more fluent style. The coding style changed and we all use screens way bigger than in the last century. A line of length 80 can easily be read with one glance still.

Alright, those are good arguments for changing to 80. Changed in the docs.

Can we remove those newlines after closing brackets?

I wonder how the Javadoc with three sentences without newline between first and second and without

around second and third sentence conforms with checkstyle. Can we rephrase those three sentences to a single sentence or separate them into one sentence and a paragraph?

I usually run ant checks before committing and it doesn't complain.

Yep, I ran that, too, and it didn't complain. Okay, removed the newlines anyway.

The three sentence javadoc will become one paragraph, but when editing it later it's easier to see, which sentence was changed.

Right, I assumed that the first paragraph should be a single sentence, because that's what goes into the summary. I didn't change this yet, but what do you think about the suggestion above?

Both 1024. and Math.floor seem unnecessary for long values. Can we change them to 1024 and omit the flooring?

Oops, please change.

Changed.

I think the preferred Git message format is: "summary of no more than 50 chars, newline, one or more parapraphs with no more than 70 chars per line". (Note: I sometimes exceed the 50, and I'm open to using something else than 70, for example, 72 which I just read on a random blog post.) Do you mind if we change the message to something like the following?

{{{ Check available disk space in relaydescs module.

Check if there's enough space before and after running the relaydescs module, and warn if less than 200 MiB are left.

Implements #18865 (moved). }}}

That's fine! And the git-msg format should have a place in the docs somewhere.

Sounds good. Amended and pushed to my task-18865 branch.

Can you add this format guideline to the docs?

So, mostly whitespace complaints. :) But I figured it's better to bring them up at some point to make future reviews faster. Again, happy to change things if you agree.

Please do. Thanks a lot!

Great! :) As soon as I know what to do with the Javadoc, I'll push and close. Thanks!

Please just rephrase to the one sentence option, than all will be visible in the summary. Thanks!

Rephrased and pushed to master. Leaving this ticket open for the Git format thing. Please either add some text for that or suggest a place where this could live and I'll add something there. Thanks!

Trac:
Status: needs_revision to assigned

Thanks!

I think the patch format for contributers is a good place, i.e. here.

Okay, added something. Closing.

Trac:
Status: assigned to closed
Resolution: N/A to fixed

closed

mentioned in issue #19830 (moved)

actively monitor resources like available storage space

Child items 0

Activity