batchReadEntries raises an unexpected exception

[mazzucchi-andrea]

BUG REPORT

Describe the bug

When I call batchReadEntries with an invalid startEntry, I get a java.util.NoSuchElementException. This happens with both WireProtocol V2 and V3.

To Reproduce

I wrote two simple tests based on the existing tests in BookKeeperTest.java

package org.apache.bookkeeper.client;

import org.apache.bookkeeper.conf.ClientConfiguration;
import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.Enumeration;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;

public class LedgerHandleReadBatchEntriesTest extends BookKeeperClusterTestCase {

    private static final Logger LOG = LoggerFactory.getLogger(LedgerHandleReadBatchEntriesTest.class);

    public LedgerHandleReadBatchEntriesTest() {
        super(5, 3, 30);
    }

    @Test
    public void testBatchReadEntriesV2() {
        ClientConfiguration conf = new ClientConfiguration().setUseV2WireProtocol(true);
        conf.setBatchReadEnabled(true);
        conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
        try (BookKeeper bkc = new BookKeeper(conf)) {
            long ledgerId;
            try (LedgerHandle lh = bkc.createLedger(3, 3, BookKeeper.DigestType.DUMMY, "password".getBytes())) {
                ledgerId = lh.getId();
                // Aggiungi 5 entry al ledger con ID da 0 a 4
                for (int i = 0; i < 5; i++) {
                    lh.addEntry(("Entry " + i).getBytes(), 0, ("Entry " + i).getBytes().length);
                }
            } catch (BKException | InterruptedException e) {
                fail("LedgerHandle inti failed: " + e.getMessage());
                return;
            }
            try (LedgerHandle lh = bkc.openLedger(ledgerId, BookKeeper.DigestType.DUMMY, "password".getBytes())) {
                Enumeration<LedgerEntry> entries = lh.batchReadEntries(-1, 5, 5 * 1024 * 1024);
                int count = 0;
                while (entries.hasMoreElements()) {
                    LedgerEntry entry = entries.nextElement();
                    assertArrayEquals(("Entry " + (count - 1)).getBytes(), entry.getEntry());
                    count++;
                }
                assertEquals(5, count);
            } catch (BKException | InterruptedException e) {
                LOG.info(e.getMessage(), e);
            }
        } catch (BKException | InterruptedException | IOException e) {
            fail("BookKeeper client init failed: " + e.getMessage());
        }
    }

    @Test
    public void testBatchReadEntriesV3() {
        ClientConfiguration conf = new ClientConfiguration();
        conf.setBatchReadEnabled(false);
        conf.setMetadataServiceUri(zkUtil.getMetadataServiceUri());
        try (BookKeeper bkc = new BookKeeper(conf)) {
            long ledgerId;
            try (LedgerHandle lh = bkc.createLedger(3, 3, BookKeeper.DigestType.DUMMY, "password".getBytes())) {
                ledgerId = lh.getId();
                // Aggiungi 5 entry al ledger con ID da 0 a 4
                for (int i = 0; i < 5; i++) {
                    lh.addEntry(("Entry " + i).getBytes(), 0, ("Entry " + i).getBytes().length);
                }
            } catch (BKException | InterruptedException e) {
                fail("LedgerHandle inti failed: " + e.getMessage());
                return;
            }
            try (LedgerHandle lh = bkc.openLedger(ledgerId, BookKeeper.DigestType.DUMMY, "password".getBytes())) {
                Enumeration<LedgerEntry> entries = lh.batchReadEntries(-1, 5, 5 * 1024 * 1024);
                int count = 0;
                while (entries.hasMoreElements()) {
                    LedgerEntry entry = entries.nextElement();
                    assertArrayEquals(("Entry " + (count - 1)).getBytes(), entry.getEntry());
                    count++;
                }
                assertEquals(5, count);
            } catch (BKException | InterruptedException e) {
                LOG.info(e.getMessage(), e);
            }
        } catch (BKException | InterruptedException | IOException e) {
            fail("BookKeeper client init failed: " + e.getMessage());
        }
    }
}

Expected behavior

It is not clear from the method description, but the other reader methods raise a BKException.BKIncorrectParameterException when provided with an invalid entry ID.

[mazzucchi-andrea]

@gulyx

[StevenLuMT]

The parameter startEntry of batchReadEntries should start from 0. You passed in -1, so it should be a direct error. Check if this is the problem.

The author of batchReadEntries is @horizonzy . Can you give some suggestions? Thank you.

[mazzucchi-andrea]

I am aware of that. A valid entryId should be equal to or greater than 0. However, batchReadEntries does not validate the input value like other methods such as readEntries. The only check performed in batchReadEntries is startEntry < lastAddConfirmed.

[StevenLuMT]

Let me understand your question. You are suggesting that batchReadEntries validates the input value like other methods such as readEntries. Is that right?

If so, I think it is a good suggestion. I will submit a PR to fix this problem.
@mazzucchi-andrea

[mazzucchi-andrea]

Yes, obviously the input validation is in the async equivalent method.

[StevenLuMT]

Good suggestion. I will submit a PR to fix this problem.

[gulyx]

@mazzucchi-andrea, why don't you propose a PR? Can you?

[mazzucchi-andrea]

Yes, I can propose a PR! I propose it in two hours. Do I introduce the new tests in BookKeeperTest.java?

[StevenLuMT]

sure,good jobs