Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

afs-client-accessd: allow configuration of export time #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marciobarbosa
Copy link
Contributor

Currently, every single host running afs-client-accessd exports the
collected data at midnight. Depending on how many hosts we have,
exporting several databases in parallel can overload the central host
and, as a result of that, substantially increase the time needed to
complete this operation.

To mitigate this problem, introduce a new configuration directive that
can be used to change the hour in which the databases should be
exported. With this directive, the number of nodes transferring data to
the central host at the same time can be reduced by setting different
export times for different hosts.

Currently, every single host running afs-client-accessd exports the
collected data at midnight. Depending on how many hosts we have,
exporting several databases in parallel can overload the central host
and, as a result of that, substantially increase the time needed to
complete this operation.

To mitigate this problem, introduce a new configuration directive that
can be used to change the hour in which the databases should be
exported. With this directive, the number of nodes transferring data to
the central host at the same time can be reduced by setting different
export times for different hosts.
Copy link
Member

@meffie meffie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest removing the filler words:

and, as a result of that,

and

To mitigate this problem,

@@ -156,6 +156,15 @@ Example:

AUDIT_PATH => '/usr/afs/logs/FileAudit',

=item EXPORT_TIME

This specifies the hour of the day (UTC) in which the *.sqlite files should be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace "should be" with "are scheduled". Should mention this is the "approximate scheduled time of day".

@@ -601,6 +611,11 @@ read_config()
$newcfg{$_} = $override{$_};
}

if ($newcfg{EXPORT_TIME} < 0 || $newcfg{EXPORT_TIME} > 24) {
$newcfg{EXPORT_TIME} = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if possible, but it would be better if we could log a warning if changing a value which is out of range.

@@ -156,6 +156,15 @@ Example:

AUDIT_PATH => '/usr/afs/logs/FileAudit',

=item EXPORT_TIME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this name might be a bit confusing, since it is not set at the "export time" but the hour of the day (in utc) the export is scheduled. Internally you've converted the value to be the epoch time by multiply the value the user gave by the number of seconds per day.

I think it would be more clear to all this EXPORT_HOUR since it is the hour the export is scheduled.


Example:

EXPORT_TIME => 16,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Export hour?

if ($newcfg{EXPORT_TIME} < 0 || $newcfg{EXPORT_TIME} > 24) {
$newcfg{EXPORT_TIME} = 0;
}
$newcfg{EXPORT_TIME} = $newcfg{EXPORT_TIME} * 60 * 60;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not convert the new config value here. If you want to do the conversion once, maybe create a new variable? If feels very side-effectly.

@@ -1230,6 +1245,27 @@ spawn_exporter()
exit(0);
}

sub
time_to_export()
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about this a bit, I think this function is not clear to me because it is doing several different things. I feel it would be more clear if we had functions which had more clear intent.

For example, for this feature we need to do the following, which can be though of as functions. Each take arguments which evaluate to one (and only one) result value.

  1. Determine the next scheduled time as a unix epoch time from the "hours" specified by the user.
    1b. To find 1, we need the value of today's midnight, as a unix epoch time.
  2. Determine if a given time is within a window of the current time (now).

In addition, we can make this more clear by declaring constant values as perl constants. That helps because it shows intent to the programmer and the interpreter which things are not going to change as the program runs.

Here's some example code (not tested) that shows what I mean.

use const SEC_PER_HOUR => 60 * 60;
use const SEC_PER_DAY => 24 * 60 * 60;
use const FLUCTUATION => 90;

# Return the previous midnight time as a unix epoch time.
sub
midnight_time()
{
   my $now = time(); 
   return $now - ($now % SEC_PER_DAY);
}

# Return the next scheduled time as a unix epoch time.
sub
schedule_time()
{
   my $hour = shift;
   return midnight_time() + ($hour * SEC_PER_HOUR);
}

# Return true if target time is within the time window.
sub
in_window()
{
    my $target = shift;
    my $now = time()

    my $range = (ALARM_INTERVAL / 2) + FLUCTUATION;
    my $lower = $now - $range;
    my $upper = $now + $range;

    return $lower < $target && $target <= $upper;
}

....

    if (...) {
       if (in_window(schedule_time($CFG{EXPORT_HOUR}))) {
                                                                                                                                        2,5           Top

@meffie
Copy link
Member

meffie commented Oct 11, 2019

Waiting for update. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants