Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

Revert "track_rotated" #180

Merged
merged 1 commit into from
Jun 10, 2021
Merged

Revert "track_rotated" #180

merged 1 commit into from
Jun 10, 2021

Conversation

djaglowski
Copy link
Member

Reverts #168

See #179

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #180 (daaeb61) into main (c46512e) will decrease coverage by 0.1%.
The diff coverage is 47.6%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #180     +/-   ##
=======================================
- Coverage   75.8%   75.6%   -0.2%     
=======================================
  Files         95      95             
  Lines       4363    4360      -3     
=======================================
- Hits        3308    3298     -10     
- Misses       733     739      +6     
- Partials     322     323      +1     
Impacted Files Coverage Δ
operator/builtin/input/file/config.go 73.4% <0.0%> (ø)
operator/builtin/input/file/reader.go 65.1% <25.0%> (-3.2%) ⬇️
operator/builtin/input/file/file.go 71.4% <60.0%> (-4.8%) ⬇️
operator/builtin/input/file/fingerprint.go 88.2% <0.0%> (+11.7%) ⬆️

@djaglowski djaglowski marked this pull request as ready for review June 10, 2021 20:40
@djaglowski djaglowski requested a review from a team June 10, 2021 20:40
@djaglowski djaglowski merged commit cc61797 into main Jun 10, 2021
@tigrannajaryan
Copy link
Member

Do we need to go back to the drawing board or we know what the right way to fix this is?

@tigrannajaryan tigrannajaryan deleted the revert-168-track_rotated branch June 11, 2021 13:52
@djaglowski
Copy link
Member Author

Do we need to go back to the drawing board or we know what the right way to fix this is?

We didn't need to go back to the drawing board, but there was some investigation required to understand the failures.

We identified two issues and addressed them on #182.

  1. The first PR (track_rotated #168) moved some code that protected against duplicated logs in Copy/Truncate scenarios. It made sense for this code to exist in the place it was moved to, but it was also needed in its original position. Thus, it is now replicated rather than moved. Performance benchmarks show trivial difference.
  2. We are clarifying that the Move/Create rotation mechanism is not supported on Windows. In theory, FILE_SHARE_DELETE mode would allow this to work, but golang does not support this. Thus, the correct action is to skip those particular tests on windows. I will document this more clearly in the design doc I am writing up.

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

Successfully merging this pull request may close these issues.

3 participants