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

ReadLedgerCommand can't get lastEntry when specifying the ledger rang… #4149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liubingxing
Copy link
Contributor

When we use the ReadLedgerCommand to read the ledger with entry range
bookkeeper shell readledger -l < ledgerId > -fe <firstEntryId> -le <lastEntryId>
This will return the entry in range [firstEntryId, lastEntryId].

If we specifying the bookie addr to read the ledger with entry range
bookkeeper shell readledger -b <bookieId> -l <ledgerId> -fe <firstEntryId> -le <lastEntryId>
It will return the entry in range [firstEntryId, lastEntryId) , the lastEntry is not included.

Because the LongStream.range will not include the lastEntry
image
image

We should replace LongStream.range with LongStream.rangeClosed
image

@zymap zymap requested a review from hangc0276 December 8, 2023 07:03
@zymap zymap added this to the 4.17.0 milestone Dec 8, 2023
Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@hangc0276 hangc0276 requested a review from zymap December 11, 2023 02:06
@hangc0276
Copy link
Contributor

rerun failure checks

1 similar comment
@hangc0276
Copy link
Contributor

rerun failure checks

@hangc0276
Copy link
Contributor

@liubingxing Would you please help take a look at the failed tests?

@liubingxing
Copy link
Contributor Author

liubingxing commented Dec 12, 2023

@liubingxing Would you please help take a look at the failed tests?

@hangc0276 Sorry, I didn't notice these failed tests, I will fix them as soon as possible.

@liubingxing
Copy link
Contributor Author

liubingxing commented Dec 12, 2023

@hangc0276
I runed the command "mvn clean package -B -nsu -DskipBookKeeperServerTests -Dorg.slf4j.simpleLogger.defaultLogLevel=INFO" in my machine, and the results were always successful;
I'm not sure what the problem is, can you help me look at this problem.
Thanks a lot.

@hangc0276
Copy link
Contributor

rerun failure checks

@hangc0276
Copy link
Contributor

The test failed with timeout

public void testWithBookieAddress() throws Exception {
ReadLedgerCommand cmd = new ReadLedgerCommand();
Assert.assertTrue(cmd.apply(bkFlags, new String[] { "-b", bookieSocketAddress.getId() }));
Assert.assertEquals(1, getMockedConstruction(NioEventLoopGroup.class).constructed().size());
Assert.assertEquals(1, getMockedConstruction(DefaultThreadFactory.class).constructed().size());
Assert.assertEquals(1, getMockedConstruction(BookieClientImpl.class).constructed().size());
verify(getMockedConstruction(NioEventLoopGroup.class).constructed().get(0), times(1)).shutdownGracefully();
verify(orderedExecutor, times(1)).shutdown();
verify(getMockedConstruction(BookieClientImpl.class).constructed().get(0), times(1)).close();
}

The failed log is

2023-12-11T06:54:35.6468730Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.LastMarkCommandTest
2023-12-11T06:54:37.4352864Z [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.787 s - in org.apache.bookkeeper.tools.cli.commands.bookie.LastMarkCommandTest
2023-12-11T06:54:37.6435604Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.ReadLedgerCommandTest
2023-12-11T07:24:37.9277456Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.SanityTestCommandTest
2023-12-11T07:24:39.8057608Z [INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.876 s - in org.apache.bookkeeper.tools.cli.commands.bookie.SanityTestCommandTest
2023-12-11T07:24:40.0306262Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.ReadLogCommandTest

@liubingxing Please take a look, thanks.

@liubingxing
Copy link
Contributor Author

The test failed with timeout

public void testWithBookieAddress() throws Exception {
ReadLedgerCommand cmd = new ReadLedgerCommand();
Assert.assertTrue(cmd.apply(bkFlags, new String[] { "-b", bookieSocketAddress.getId() }));
Assert.assertEquals(1, getMockedConstruction(NioEventLoopGroup.class).constructed().size());
Assert.assertEquals(1, getMockedConstruction(DefaultThreadFactory.class).constructed().size());
Assert.assertEquals(1, getMockedConstruction(BookieClientImpl.class).constructed().size());
verify(getMockedConstruction(NioEventLoopGroup.class).constructed().get(0), times(1)).shutdownGracefully();
verify(orderedExecutor, times(1)).shutdown();
verify(getMockedConstruction(BookieClientImpl.class).constructed().get(0), times(1)).close();
}

The failed log is

2023-12-11T06:54:35.6468730Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.LastMarkCommandTest
2023-12-11T06:54:37.4352864Z [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.787 s - in org.apache.bookkeeper.tools.cli.commands.bookie.LastMarkCommandTest
2023-12-11T06:54:37.6435604Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.ReadLedgerCommandTest
2023-12-11T07:24:37.9277456Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.SanityTestCommandTest
2023-12-11T07:24:39.8057608Z [INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.876 s - in org.apache.bookkeeper.tools.cli.commands.bookie.SanityTestCommandTest
2023-12-11T07:24:40.0306262Z [INFO] Running org.apache.bookkeeper.tools.cli.commands.bookie.ReadLogCommandTest

@liubingxing Please take a look, thanks.

@hangc0276 Thanks for pointing out the error, I updated the code. Please review again.

@eolivelli eolivelli modified the milestones: 4.17.0, 4.18.0 Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants