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.

Sign up for GitHub

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

Jump to bottom

Let the max depth for daily log listener deletion be configurable. #583

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

Let the max depth for daily log listener deletion be configurable. #583

wants to merge 1 commit into from

Conversation

alcarraz
Copy link
Contributor

@alcarraz alcarraz commented Feb 8, 2024

Fixes #580 About the rotation of q2 logs

@alcarraz alcarraz marked this pull request as draft February 8, 2024 22:42
@alcarraz
Copy link
Contributor Author

alcarraz commented Feb 8, 2024

It seems I will have to change tests to work on windows, marked as drat until then.

@alcarraz alcarraz force-pushed the jPOS-580-allow-configure-max-depth-deletion-in-daily-log-listener branch 3 times, most recently from aa6f836 to be6ebde Compare February 10, 2024 21:45
@alcarraz
Let the max depth for deletion be configurable.
503ed08 
Signed-off-by: Andrés Alcarraz <[email protected]>

jpos#580 About the rotation of q2 logs
@alcarraz alcarraz force-pushed the jPOS-580-allow-configure-max-depth-deletion-in-daily-log-listener branch from be6ebde to 503ed08 Compare February 10, 2024 22:09
@alcarraz alcarraz marked this pull request as ready for review February 10, 2024 22:19
@alcarraz alcarraz changed the title Let the max depth for deletion be configurable. Let the max depth for daily log listener deletion be configurable. Feb 13, 2024
@alcarraz
Copy link
Contributor Author

Sorry for all the noise regarding this PR, wasn't understanding the real problem behind the temporary directory cleanup on windows, the last failure (java 11, windows-latest) doesn't seem to be related to this PR, but I don't have the permissions to rerun the workflow.

@ar
Copy link
Member

ar commented Feb 14, 2024

I have mixed feelings about this PR. Please see this unanswered comment:

#580 (comment)

@alcarraz
Copy link
Contributor Author

I have mixed feelings about this PR. Please see this unanswered comment:

#580 (comment)

Yes, I get your concerns, I did this based on the part of your comment that states:

Setting DEF_MAXDEPTH to 1 as the default limit for recursive deletion operations provides a safer approach, but I agree with you that this should be configurable for situations where you know what you're doing.

How would you feel about renaming the constant (it's not a default if it is cannot be overridden) or add a getter for max depth that can be overridden by a subclass? This way, it's less likely the user doesn't know what he is doing.

@ar
Copy link
Member

ar commented Feb 14, 2024

I'd like an answer to my question in the issue, if the problem was the final / then there's nothing to fix. I can't imagine a situation where we want to traverse a deep directory hierarchy to find logs to delete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

About the rotation of q2 logs
2 participants
@alcarraz @ar