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

invert order of whisper-merge #10

Closed
wants to merge 1 commit into from

Conversation

slackhappy
Copy link
Contributor

I ran into a bug when running a backfill
using whisper merge. The problem is that it
chooses the longest (in terms of timespan)
archive, and works its way to the shortest.

This prevents longer, less granular archives from being
filled in using the aggregationMethod as applied
to the shorter, more granular archives.

There was also a subtle bug that could occur if the real
clock changes while the merge is in progress. This
could cause the wrong archive to be picked.

This change also protects the user from merge scenarios
that haven't been carefully considered. Clearly,
some merge scenarios between differing archive
configurations are valid, but careful analysis
is important.

I ran into a bug when running a backfill
using whisper merge.  The problem is that it
chooses the longest (in terms of timespan)
archive, and works its way to the shortest.

This prevents longer, less granular archives from being
filled in using the aggregationMethod as applied
to the shorter, more granular archives.

There was also a subtle bug that could occur if the real
clock changes while the merge is in progress.  This
could cause the wrong archive to be picked.

This change also protects the user from merge scenarios
that haven't been carefully considered.  Clearly,
some merge scenarios between differing archive
configurations are valid, but careful analysis
is important.
mleinart added a commit that referenced this pull request Nov 23, 2012
Closes pull request #10

Enforce merges to be strictly between two whisper files with the same
archive configuration and do a direct archive copy. This solves the
issue of time changing between archive copies.
@mleinart
Copy link
Member

Hey, thanks for the pull. I've kindof merged this - I like the take on explicitly preventing merges from unalike whisper files, but I'm not crazy about adding the now param to the public api though it is the simplest way. The plus in restricting it in this way is that we can now explicit archive-by-archive copy rather than relying on the mechanics of whisper reads which aren't well suited to utility work. Reading from the archive outside of fetch() fixes the problem with 'now' changing and I think this'll set it up to more easily do complex merges.

lmk if you see any issue with the changes I made

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.

None yet

2 participants