Accumulo
  1. Accumulo
  2. ACCUMULO-3

MockConnector should return a MockDeleter

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4.0
    • Component/s: client
    • Labels:
      None

      Description

      The MockConnector currently only returns MockScanners, MockWriters, MockMultiTableMockWriter. However, for real testing of Accumulo, we need to have a real deleter as well.

      There were rumblings that the BatchDelete mechanism would be removed, but in the mean time, we should still support it because at the very least it breaks testing.

      1. java_ACCUMULO-3_v3.txt
        7 kB
        Jesse Yates
      2. java_ACCUMULO_3_v2.txt
        7 kB
        Jesse Yates
      3. java_ACCUMULO-3.txt
        6 kB
        Jesse Yates

        Activity

        Hide
        Jesse Yates added a comment -

        I'm working on a patch for this - shouldn't be too long to get it up.

        Show
        Jesse Yates added a comment - I'm working on a patch for this - shouldn't be too long to get it up.
        Hide
        Jesse Yates added a comment -

        Adding patch for adding MockDeleter. Also updated MockConnectorTest to test using the MockDeleter.

        Show
        Jesse Yates added a comment - Adding patch for adding MockDeleter. Also updated MockConnectorTest to test using the MockDeleter.
        Hide
        Billie Rinaldi added a comment -

        We may want to keep the existing testDelete method along with a new test method for the MockBatchDeleter. Perhaps we could check to see if removing testDelete changes the code coverage.

        Show
        Billie Rinaldi added a comment - We may want to keep the existing testDelete method along with a new test method for the MockBatchDeleter. Perhaps we could check to see if removing testDelete changes the code coverage.
        Hide
        Jesse Yates added a comment -

        That's fair. Here's the numbers from running ECL Emma in Eclipse on accumulo-core:

        Before:
        Covered: 24.2%, 84525 lines
        Misses: 264316

        After:
        Covered: 24.2%, 84556 lines
        Missed: 264341 lines

        Which honestly doesn't tell the full story. I don't think it would be that bad to have both tests. Still want me to add it back in?

        Show
        Jesse Yates added a comment - That's fair. Here's the numbers from running ECL Emma in Eclipse on accumulo-core: Before: Covered: 24.2%, 84525 lines Misses: 264316 After: Covered: 24.2%, 84556 lines Missed: 264341 lines Which honestly doesn't tell the full story. I don't think it would be that bad to have both tests. Still want me to add it back in?
        Hide
        Billie Rinaldi added a comment -

        Yes, I would keep both tests. Also, you should add some data and delete it in the test. I would suggest changing the setRanges to something like the following: deleter.setRanges(Collections.singletonList(new Range("r1"))); this will range over the entire row r1 (you don't have to specify "r1" twice). It probably doesn't matter for a test, but I think Collections.singletonList is more efficient than Arrays.asList.

        I was experimenting with it a little, and found unexpected behavior when you do a deleter.delete() without first calling setRanges on the deleter. The real BatchScanner (TabletServerBatchReader) throws IllegalStateException("ranges not set") when you get an iterator from it without setting ranges, but the MockBatchScanner does not.

        Show
        Billie Rinaldi added a comment - Yes, I would keep both tests. Also, you should add some data and delete it in the test. I would suggest changing the setRanges to something like the following: deleter.setRanges(Collections.singletonList(new Range("r1"))); this will range over the entire row r1 (you don't have to specify "r1" twice). It probably doesn't matter for a test, but I think Collections.singletonList is more efficient than Arrays.asList. I was experimenting with it a little, and found unexpected behavior when you do a deleter.delete() without first calling setRanges on the deleter. The real BatchScanner (TabletServerBatchReader) throws IllegalStateException("ranges not set") when you get an iterator from it without setting ranges, but the MockBatchScanner does not.
        Hide
        Jesse Yates added a comment -

        doesn't matter for a test, but I think Collections.singletonList is more efficient than Arrays.asList.

        Yeah, that is really miniscule - difference is the allocation of 1 element array and a 10 element array (assuming the Harmony source code is pretty close to the actual), though I like the former for expressing the actual intent.

        Don't know why I was specifying both ends of the range - total oversight.

        Working on on new patch.

        I was experimenting with it a little, and found unexpected behavior when you do a deleter.delete() without first calling setRanges on the deleter. The real BatchScanner (TabletServerBatchReader) throws IllegalStateException("ranges not set") when you get an iterator from it without setting ranges, but the MockBatchScanner does not.

        I think that should be handled in a separate patch, though I agree its a problem. Want me to open a new ticket?

        Your point also ties back into the bigger issue of the mock instances lagging behind the actual implementation (ACCUMULO-14).

        Show
        Jesse Yates added a comment - doesn't matter for a test, but I think Collections.singletonList is more efficient than Arrays.asList. Yeah, that is really miniscule - difference is the allocation of 1 element array and a 10 element array (assuming the Harmony source code is pretty close to the actual), though I like the former for expressing the actual intent. Don't know why I was specifying both ends of the range - total oversight. Working on on new patch. I was experimenting with it a little, and found unexpected behavior when you do a deleter.delete() without first calling setRanges on the deleter. The real BatchScanner (TabletServerBatchReader) throws IllegalStateException("ranges not set") when you get an iterator from it without setting ranges, but the MockBatchScanner does not. I think that should be handled in a separate patch, though I agree its a problem. Want me to open a new ticket? Your point also ties back into the bigger issue of the mock instances lagging behind the actual implementation ( ACCUMULO-14 ).
        Hide
        Jesse Yates added a comment -

        Uploading new version: adding back in original delete test and adding in more testing for the batch deleter.

        Show
        Jesse Yates added a comment - Uploading new version: adding back in original delete test and adding in more testing for the batch deleter.
        Hide
        Billie Rinaldi added a comment -

        In the new test, I think you used the "testDelete" method instead of the new "checkDeleted" method. You may want to pass an int into checkDeleted that has the expected count. The last test in testDeletewithBatchDeleter leaves one entry in the table.

        Show
        Billie Rinaldi added a comment - In the new test, I think you used the "testDelete" method instead of the new "checkDeleted" method. You may want to pass an int into checkDeleted that has the expected count. The last test in testDeletewithBatchDeleter leaves one entry in the table.
        Hide
        Jesse Yates added a comment -

        Working on this - didn't see your post come up with notifications not working.

        Show
        Jesse Yates added a comment - Working on this - didn't see your post come up with notifications not working.
        Hide
        Jesse Yates added a comment -

        Hmm, it seems like the checkDeleted method isn't present anymore (checked both the svn and github) on MockConnectorTest, though I don't see a patch changing that anywhere, though it was definitely present in older versions (e.g. the first svn checkout).

        Am I losing my mind or is that right? If I'm still sane, I'll just throw the check into the test method and then open a ticket to refactor the whole test to use the check.

        Show
        Jesse Yates added a comment - Hmm, it seems like the checkDeleted method isn't present anymore (checked both the svn and github) on MockConnectorTest, though I don't see a patch changing that anywhere, though it was definitely present in older versions (e.g. the first svn checkout). Am I losing my mind or is that right? If I'm still sane, I'll just throw the check into the test method and then open a ticket to refactor the whole test to use the check.
        Hide
        Billie Rinaldi added a comment -

        The checkDeleted method is in your second patch; there are +s next to it. It doesn't appear to have been in any older svn versions.

        Show
        Billie Rinaldi added a comment - The checkDeleted method is in your second patch; there are +s next to it. It doesn't appear to have been in any older svn versions.
        Hide
        Jesse Yates added a comment - - edited

        Ok, im clearly losing my mind. Sorry about about.

        You are right - my bad

        Show
        Jesse Yates added a comment - - edited Ok, im clearly losing my mind. Sorry about about. You are right - my bad
        Hide
        Jesse Yates added a comment -

        Making changes to use checkDeleted (renamed) rather than testDelete (dumb mistake).

        Show
        Jesse Yates added a comment - Making changes to use checkDeleted (renamed) rather than testDelete (dumb mistake).
        Hide
        Billie Rinaldi added a comment -

        Accidentally checked in the patch without a comment. http://svn.apache.org/viewvc?rev=1185873&view=rev

        Show
        Billie Rinaldi added a comment - Accidentally checked in the patch without a comment. http://svn.apache.org/viewvc?rev=1185873&view=rev

          People

          • Assignee:
            Jesse Yates
            Reporter:
            Jesse Yates
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development