Accumulo
  1. Accumulo
  2. ACCUMULO-2232

Combiners can cause deleted data to come back

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: client, tserver
    • Labels:
      None

      Description

      The case-
      3 files with-

      • 1 with a key, k, with timestamp 0, value 3
      • 1 with a delete of k with timestamp 1
      • 1 with k with timestamp 2, value 2

      The column of k has a summing combiner set on it. The issue here is that depending on how the major compactions play out, differing values with result. If all 3 files compact, the correct value of 2 will result. However, if 1 & 3 compact first, they will aggregate to 5. And then the delete will fall after the combined value, resulting in the result 5 to persist.

      First and foremost, this should be documented. I think to remedy this, combiners should only be used on full MajC, not not full ones. This may necessitate a special flag or a new combiner that implemented the proper semantics.

        Issue Links

          Activity

          Hide
          Christopher Tubbs added a comment -

          This issue would probably go away entirely, if we separated the Combiner from the scope it's supposed to run at, and made the "full major compaction" scope a top-level iterator scope, along with "partial major compaction". Obviously, this is not a simple transition..., but I think it's an (eventual) worthwhile one.

          Show
          Christopher Tubbs added a comment - This issue would probably go away entirely, if we separated the Combiner from the scope it's supposed to run at, and made the "full major compaction" scope a top-level iterator scope, along with "partial major compaction". Obviously, this is not a simple transition..., but I think it's an (eventual) worthwhile one.
          Hide
          Keith Turner added a comment -

          Another possible way to handle efficiency concerns is to have Accumulo initiate a major compaction if scans are repeatedly combining a lot of data. This serves as a use case for ACCUMULO-1266.

          Show
          Keith Turner added a comment - Another possible way to handle efficiency concerns is to have Accumulo initiate a major compaction if scans are repeatedly combining a lot of data. This serves as a use case for ACCUMULO-1266 .
          Hide
          Adam Fuchs added a comment - - edited

          I agree with Josh (and everyone else) on both counts: the performance
          implications will be huge and this is enough rope for people to hang
          themselves with. However, I think a lot of people use combiners on tables
          that are append-only and never delete (at least not a record at a time).
          The warning unsafe doom will ensue bypass is pretty important to support
          those uses, but I also think it is best to default to accuracy while we
          implement a better fix.

          It seems like the right way to fix this in the long run is to keep track of
          timestamp ranges of files and calculate two properties on the set of files
          being compacted:
          1. Is the time range contiguous, or are there other files not being
          compacted that overlap the range?
          2. Are there any files with an older timestamp?
          This way we can run combiners on any compactions that satisfy property #1,
          and preserve the most recent deletes in any compaction that satisfies
          property #2. This generally makes minor compactions safe for running
          combiners (assuming Accumulo sets the timestamps and there is no bulk
          loading), although the most recent delete needs to be preserved. If I were
          to speculate about general major compactions, I would say that when splits
          are rare most other compactions also have property #1.

          I think we could expose these properties in the iterator environment. We
          could even come up with a compaction strategy that biases compactions
          towards contiguous time ranges if we were ambitious.

          Show
          Adam Fuchs added a comment - - edited I agree with Josh (and everyone else) on both counts: the performance implications will be huge and this is enough rope for people to hang themselves with. However, I think a lot of people use combiners on tables that are append-only and never delete (at least not a record at a time). The warning unsafe doom will ensue bypass is pretty important to support those uses, but I also think it is best to default to accuracy while we implement a better fix. It seems like the right way to fix this in the long run is to keep track of timestamp ranges of files and calculate two properties on the set of files being compacted: 1. Is the time range contiguous, or are there other files not being compacted that overlap the range? 2. Are there any files with an older timestamp? This way we can run combiners on any compactions that satisfy property #1, and preserve the most recent deletes in any compaction that satisfies property #2. This generally makes minor compactions safe for running combiners (assuming Accumulo sets the timestamps and there is no bulk loading), although the most recent delete needs to be preserved. If I were to speculate about general major compactions, I would say that when splits are rare most other compactions also have property #1. I think we could expose these properties in the iterator environment. We could even come up with a compaction strategy that biases compactions towards contiguous time ranges if we were ambitious.
          Hide
          Josh Elser added a comment -

          I'm a little worried about implications (sorry for using that phrase) that only running combiners on full MajC would have on performance since, for heavy combination, you're going to be persisting and later re-reading many records instead of just once for a potentially very long time (if you assume that full MajCs are few and far between).

          I can't come up with another easy way to fix it though for the SummingCombiner example, so accuracy is still better than being slow. Anything else I can think of would involve persisting deletes across non-full compactions which would require quite a bit more work to get correct, I imagine.

          Show
          Josh Elser added a comment - I'm a little worried about implications (sorry for using that phrase) that only running combiners on full MajC would have on performance since, for heavy combination, you're going to be persisting and later re-reading many records instead of just once for a potentially very long time (if you assume that full MajCs are few and far between). I can't come up with another easy way to fix it though for the SummingCombiner example, so accuracy is still better than being slow. Anything else I can think of would involve persisting deletes across non-full compactions which would require quite a bit more work to get correct, I imagine.
          Hide
          Sean Busbey added a comment -

          If there is a special flag that allows running the combiner for partial compactions, it could fail if it detects a delete. However there are other ways to delete and this sanity check would not cover the case of row deletion, row filtering, etc.

          I say a flag to allow running on partial compactions is reasonable, provided it comes with a strong warning about not seeing all rows (calling out this specific issue).

          In that case, I think it should be up to the combiner to handle logic around deletes. I can think of combiner applications where it would make sense not to fail when there's a delete.

          Seems like this should be fixed in 1.4, 1.5, and 1.6.

          +1, seems like an oversight that combiners didn't default to full majc

          Show
          Sean Busbey added a comment - If there is a special flag that allows running the combiner for partial compactions, it could fail if it detects a delete. However there are other ways to delete and this sanity check would not cover the case of row deletion, row filtering, etc. I say a flag to allow running on partial compactions is reasonable, provided it comes with a strong warning about not seeing all rows (calling out this specific issue). In that case, I think it should be up to the combiner to handle logic around deletes. I can think of combiner applications where it would make sense not to fail when there's a delete. Seems like this should be fixed in 1.4, 1.5, and 1.6. +1, seems like an oversight that combiners didn't default to full majc
          Hide
          Keith Turner added a comment -

          I think to remedy this, combiners should only be used on full MajC, not not full ones.

          +1

          This may necessitate a special flag or a new combiner that implemented the proper semantics.

          Why not just modify current combiner to only run on full majc?

          If there is a special flag that allows running the combiner for partial compactions, it could fail if it detects a delete. However there are other ways to delete and this sanity check would not cover the case of row deletion, row filtering, etc.

          Seems like this should be fixed in 1.4, 1.5, and 1.6.

          Show
          Keith Turner added a comment - I think to remedy this, combiners should only be used on full MajC, not not full ones. +1 This may necessitate a special flag or a new combiner that implemented the proper semantics. Why not just modify current combiner to only run on full majc? If there is a special flag that allows running the combiner for partial compactions, it could fail if it detects a delete. However there are other ways to delete and this sanity check would not cover the case of row deletion, row filtering, etc. Seems like this should be fixed in 1.4, 1.5, and 1.6.
          Hide
          Josh Elser added a comment -

          I've seen this one happen before, and it can be rather surprising.

          Definitely on the documentation. Interesting idea about usage on full MajC.

          Show
          Josh Elser added a comment - I've seen this one happen before, and it can be rather surprising. Definitely on the documentation. Interesting idea about usage on full MajC.

            People

            • Assignee:
              Unassigned
              Reporter:
              John Vines
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:

                Development