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

[FLUME-3352]:Unnecessary canary test will block on readonly spooldir … #314

Merged
merged 2 commits into from Feb 27, 2020
Merged

[FLUME-3352]:Unnecessary canary test will block on readonly spooldir … #314

merged 2 commits into from Feb 27, 2020

Conversation

hackty
Copy link
Contributor

@hackty hackty commented Jan 16, 2020

@hackty hackty requested review from bessbd and mpercy January 16, 2020 12:43
@hackty
Copy link
Contributor Author

hackty commented Jan 16, 2020

Could you please review? @bessbd @mpercy

@bessbd
Copy link
Member

bessbd commented Jan 18, 2020

Hi @hackty !

Thank you for this patch! On a first read, it looks good. Can you please add a test to ensure it works correctly and to avoid future changes accidentally removing this fix?

@hackty
[FLUME-3352]: add unit tests to ensure TrackingPolicy.RENAME / Tracki…
2d44f64 
…ngPolicy.TRACKER_DIR effective on readonly spooldir
@hackty
Copy link
Contributor Author

hackty commented Jan 19, 2020 •
edited

@bessbd Thanks for your prompt reply. Two unit tests were added to ensure the correctness on the fix.

The description of two unit test:
testRenameTrackingPolicyOnReadonlySpoolDirectory
One supplemented to examine the mechanisation of canary (RenameTrackingPolicy is enabled so the spooldir should have the write access.)
It can pass no matter before or after the fix.

testTrackerDirTrackingPolicyOnReadonlySpoolDirectory
One that added to ensure the correctness on currect fix.(TrackerDirTrackingPolicy is enabled and the trackdir is different from spooldir. This time the write permission of spooldir is not necessary to the trackdir and also works well.)
It can fail before the fix and pass after the fix.

@hackty
Copy link
Contributor Author

hackty commented Jan 23, 2020

Could you please review the tests? @bessbd

@bessbd
Copy link
Member

bessbd commented Jan 26, 2020

Thank you for the tests and the descriptions, @hackty .
It looks good to me. I'll leave some time for @mpercy to add his comments.

@hackty hackty requested review from tmgstevens and removed request for tmgstevens February 16, 2020 05:16
@mpercy
Copy link
Contributor

mpercy commented Feb 27, 2020

Patch looks good to me, tests pass. Thanks for the contribution and your patience and persistence in getting this reviewed, @hackty!

@mpercy mpercy merged commit 5c9bfe6 into apache:trunk Feb 27, 2020
tmgstevens pushed a commit to tmgstevens/flume that referenced this pull request May 21, 2020
@hackty
FLUME-3352: Skip spooldir canary when tracker dir is used (apache#314)
49921f9 
* Unnecessary canary test will block on readonly spooldir while another trackerdir is set.
* Add unit tests to ensure TrackingPolicy.RENAME / TrackingPolicy.TRACKER_DIR effective on readonly spooldir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpercy mpercy mpercy approved these changes

@bessbd bessbd Awaiting requested review from bessbd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@hackty @bessbd @mpercy