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

FDG-6647 Made Wait Interval of Execution Logs Cleaner into Config Variable #9

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

Conversation

lakshmi-kmk
Copy link

…iabls

cleanExecutionLogs();
lastLogCleanTime = currentTime;
}

wait(CLEANER_THREAD_WAIT_INTERVAL_MS);
wait(cleanerThreadCustomWaitIntervalMs);
} catch (InterruptedException e) {
logger.info("Interrupted. Probably to shut down.");
}

Choose a reason for hiding this comment

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

If possible, please add UTs

@@ -66,6 +66,8 @@
"azkaban.executorinfo.refresh.maxThreads";
private static final String AZKABAN_MAX_DISPATCHING_ERRORS_PERMITTED =
"azkaban.maxDispatchingErrors";
private static final String AZKABAN_WEBSERVER_CLEANER_WAIT_INTERVAL =
"azkaban.webserver.cleaner.waitInterval.ms";

Choose a reason for hiding this comment

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

azkaban is following this kind of notion: azkaban.activeexecutor.refresh.milisecinterval. we can follow similar: azkaban.webserver.cleaner.wait.milisecinterval

Copy link
Author

Choose a reason for hiding this comment

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

changed

@@ -1689,12 +1691,14 @@ public int getExecutableFlows(int projectId, String flowId, int from,
24 * 60 * 60 * 1000;

private final long executionLogsRetentionMs;
private final long cleanerThreadCustomWaitIntervalMs;

Choose a reason for hiding this comment

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

you can name it cleanerThreadWaitIntervalMs only, it is either default or custom

Copy link
Author

Choose a reason for hiding this comment

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

changed

Copy link

@mudit1289 mudit1289 left a comment

Choose a reason for hiding this comment

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

LGTM

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