peter-lawrey / Java-Chronicle

Java Indexed Record Chronicle
1.22k stars 193 forks source link

Bug and/or unexpected behaviour of hasNextIndex() #25

Closed kenota closed 11 years ago

kenota commented 11 years ago

I started playing with Chronicle library and trying to start from basic stuff, like walking through saved Chronicle. I encountered some problems with Excerpt.hasNextIndex() and Excerpt.nextIndex() methods. I believe there are two cases, one is bug and another is either me not understanding what hasNextIndex() should do.

  1. First is, I think, a bug. If you open IndexedChronicle and create excerpt on it, hasNextIndex() will return false, although nextIndex() will return true. If you change order, both will return true (since index was moed to 0). See test methods testHasNextIndexFail and testHasNextIndexPass in attached unit test
  2. Second problem comes when I try to iterate over Chronicle. Being used to Iterator semantics of hasNext() and next() methods, I would expect nextIndex() method to return true whenever hasNextIndex() returns true. But it seems that this is not the case with Excerpt. See testHasNextIndexIteration() method in unit test.

Unit test to reproduce bug and show chronicle iteration:

package com.binarybuffer.chronicle.test;

import com.higherfrequencytrading.chronicle.Chronicle;
import com.higherfrequencytrading.chronicle.Excerpt;
import com.higherfrequencytrading.chronicle.impl.IndexedChronicle;
import com.higherfrequencytrading.chronicle.tools.ChronicleTools;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

import java.io.File;
import java.io.IOException;
import static org.junit.Assert.*;
/**
 * Test correct behaviour of hasNextIndex() and nextIndex() methods
 */
public class ExcerptHasNextTest {
    static final String TMP = System.getProperty("java.io.tmpdir");
    public static final int NUMBER_OF_ENTRIES = 12;

    @Test
    public void testHasNextIndexFail() throws IOException {
        final Chronicle chr = createChronicle("hasNextFail");

        final Excerpt readExcerpt = chr.createExcerpt();
        assertTrue("Read excerpt should have next index", readExcerpt.hasNextIndex());
        assertTrue("It should be possible to move to next index", readExcerpt.nextIndex());
    }

    @Test
    public void testHasNextIndexPass() throws IOException {
        final Chronicle chr = createChronicle("hasNextPass");

        final Excerpt readExcerpt = chr.createExcerpt();
        assertTrue("It should be possible to move to next index", readExcerpt.nextIndex());
        assertTrue("Read excerpt should have next index", readExcerpt.hasNextIndex());
    }

    @Test
    public void testHasNextIndexIteration() throws IOException {
        final Chronicle chr = createChronicle("testIteration");

        final Excerpt readExcerpt = chr.createExcerpt();
        readExcerpt.index(0);

        while (readExcerpt.hasNextIndex()) {
            assertTrue("I would expect nextIndex() return true after hasNextIndex() returns true",
                    readExcerpt.nextIndex());
        }
    }

    private Chronicle createChronicle(String name) throws IOException {
        final String basePath = TMP + File.separator + name;
        final IndexedChronicle chr = new IndexedChronicle(basePath);

        ChronicleTools.deleteOnExit(basePath);

        final Excerpt excerpt = chr.createExcerpt();

        for (int i = 0; i < NUMBER_OF_ENTRIES; ++i) {
            excerpt.startExcerpt(128);
            excerpt.writeBytes("test");
            excerpt.finish();
        }

        assertTrue("Chronicle should hold all values", NUMBER_OF_ENTRIES == excerpt.size());

        return chr;
    }
}
peter-lawrey commented 11 years ago

Thank you for the unit tests. There was a simple bug which meant hasNextIndex() didn't really do anything useful. :| I have fixed it now and all these tests pass.

jbrisbin commented 11 years ago

I think I'm seeing a similar problem. Is this code included in a specific version available on Maven?

peter-lawrey commented 11 years ago

If memory serves me correctly, it should be fixed in Chronicle 1.7.1 The latest is 1.7.2. I suggest you use this version.

On 29 July 2013 15:57, Jon Brisbin notifications@github.com wrote:

I think I'm seeing a similar problem. Is this code included in a specific version available on Maven?

— Reply to this email directly or view it on GitHubhttps://github.com/peter-lawrey/Java-Chronicle/issues/25#issuecomment-21721373 .

jbrisbin commented 11 years ago

I was using 1.7.2.

It's odd because I'm seeing different behaviour on the Linux CI servers than I am on my Mac OS X local dev box. The hasNextIndex() is correctly reporting false on my local tests, but does not report false on the CI server build. This causes the tests to hang or fail completely.

peter-lawrey commented 11 years ago

Can you push me a unit test to demonstrate this?

On 29 July 2013 16:13, Jon Brisbin notifications@github.com wrote:

I was using 1.7.2.

It's odd because I'm seeing different behaviour on the Linux CI servers than I am on my Mac OS X local dev box. The hasNextIndex() is correctly reporting false on my local tests, but does not report false on the CI server build. This causes the tests to hang or fail completely.

— Reply to this email directly or view it on GitHubhttps://github.com/peter-lawrey/Java-Chronicle/issues/25#issuecomment-21722414 .

peter-lawrey commented 11 years ago

BTW I don't have access to a Mac OSX machine but I should be able to get it working for Linux.

On 29 July 2013 16:17, Peter Lawrey peter.lawrey@gmail.com wrote:

Can you push me a unit test to demonstrate this?

On 29 July 2013 16:13, Jon Brisbin notifications@github.com wrote:

I was using 1.7.2.

It's odd because I'm seeing different behaviour on the Linux CI servers than I am on my Mac OS X local dev box. The hasNextIndex() is correctly reporting false on my local tests, but does not report false on the CI server build. This causes the tests to hang or fail completely.

— Reply to this email directly or view it on GitHubhttps://github.com/peter-lawrey/Java-Chronicle/issues/25#issuecomment-21722414 .