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

create RP branch to use as local master per PR comment #2

Open
wants to merge 2 commits into
base: rubiconproject
Choose a base branch
from

Conversation

nerussell
Copy link

set local default to 5 days of log retention

I do not seem to have privilege to set 'rubiconproject' branch as the git default.

set local default to 5 days of log retention
.gitignore Outdated
# PyBuilder
target/
## Directory-based project format:
.idea/

Choose a reason for hiding this comment

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

I believe this should cover all of the contents of .idea/ below

Choose a reason for hiding this comment

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

I'd rather keep this as close as possible to the original. I think most of this stuff does not apply to this project. You can move it to a .gitignore_global if you need it for your own dev environment.

Copy link
Author

Choose a reason for hiding this comment

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

fixed in next commit

@@ -20,7 +20,7 @@
SCHEDULE_INTERVAL = "@daily" # How often to Run. @daily - Once a day at Midnight
DAG_OWNER_NAME = "operations" # Who is listed as the owner of this DAG in the Airflow Web Server
ALERT_EMAIL_ADDRESSES = [] # List of email address to send email alerts to if this job fails
DEFAULT_MAX_LOG_AGE_IN_DAYS = 30 # Length to retain the log files if not already provided in the conf. If this is set to 30, the job will remove those files that are 30 days old or odler
DEFAULT_MAX_LOG_AGE_IN_DAYS = 5 # Length to retain the log files if not already provided in the conf. If this is set to 30, the job will remove those files that are 30 days old or odler

Choose a reason for hiding this comment

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

Is it possible to use the maxLogAgeInDays config option instead of changing this?

Copy link
Author

@nerussell nerussell Nov 27, 2017

Choose a reason for hiding this comment

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

perhaps, but I don't know how to do that. The way this is written requires passing a command line arg into airflow itself. The comments show a manual invocation:

airflow trigger_dag --conf '{"maxLogAgeInDays":30}' airflow-log-cleanup

would that perhaps be done by putting a value into the "default_args" object?

Choose a reason for hiding this comment

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

The above airflow cli command should work but it means having another service run on a timer (or yet another DAG to invoke a DAG).

Choose a reason for hiding this comment

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

What do you think about the "another DAG to invoke a DAG" approach?

Copy link
Author

Choose a reason for hiding this comment

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

I'd rather convert this to a systemd job than create more DAGs to manage DAGs to manage log files. The way the airflow vars are defined and passed seems to change frequently. The --conf mechanism appears to have be introduced as a partial response to this exact issue.

Copy link
Author

Choose a reason for hiding this comment

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

we could rewrite this DAG to pass in a more standard "params" object I think.

@drstevens
Copy link

I updated the default repo branch

@Benaiad Benaiad removed their request for review May 18, 2019 10:34
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.

3 participants