You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "horizonzy (via GitHub)" <gi...@apache.org> on 2023/08/02 00:41:10 UTC
[GitHub] [bookkeeper] horizonzy opened a new pull request, #4038: Improve DefaultEntryLogger read performance.
horizonzy opened a new pull request, #4038:
URL: https://github.com/apache/bookkeeper/pull/4038
If the BufferChannel is BufferedReadChannel, the size won't change. We can cache the fileSize to avoid time-consuming operation.
### Performance tests.
The entry log `f` file holds 1GB data.
```
-rw-r--r--@ 1 horizon staff 1.0G 7 26 17:47 f.log
```
#### 1. Test DefaultEntryLogger.scanEntryLog (10x performance improvement)
```
public static void main(String[] args) throws IOException {
ServerConfiguration serverConfiguration = new ServerConfiguration();
serverConfiguration.setLedgerDirNames(new String[]{"/Users/horizon/Downloads/bk1/bk-data"});
DefaultEntryLogger defaultEntryLogger = new DefaultEntryLogger(serverConfiguration);
long l = System.currentTimeMillis();
AtomicInteger entryCount = new AtomicInteger();
defaultEntryLogger.scanEntryLog(15, new EntryLogScanner() {
@Override
public boolean accept(long ledgerId) {
return true;
}
@Override
public void process(long ledgerId, long offset, ByteBuf entry) throws IOException {
entryCount.incrementAndGet();
}
});
long spend = System.currentTimeMillis() - l;
System.out.println("Scan entry end, entry count: " + entryCount.get());
System.out.println("Spend: " + spend);
}
```
Before this pr:
```
Scan entry end, entry count: 17886769
Spend: 37273
```
After this pr:
```
Scan entry end, entry count: 17886769
Spend: 3363
```
#### 2. Test DefaultEntryLogger.readFromLogChannel (30x performance improvement)
```
public static void main(String[] args) throws IOException {
ServerConfiguration serverConfiguration = new ServerConfiguration();
serverConfiguration.setLedgerDirNames(new String[]{"/Users/horizon/Downloads/bk1/bk-data"});
DefaultEntryLogger defaultEntryLogger = new DefaultEntryLogger(serverConfiguration);
long l = System.currentTimeMillis();
BufferedReadChannel channel = defaultEntryLogger.getChannelForLogId(15);
int pos = 0;
//Read total 1gb data from the BufferedReadChannel.
long gb = 1024 * 1024 * 1024;
ByteBuf byteBuf = ByteBufAllocator.DEFAULT.heapBuffer(10);
while (pos < gb) {
defaultEntryLogger.readFromLogChannel(15, channel, byteBuf, 0);
pos += byteBuf.readableBytes();
byteBuf.clear();
}
long spend = System.currentTimeMillis() - l;
System.out.println("Spend: " + spend);
}
```
Before this pr:
```
Spend: 69810
```
After this pr:
```
Spend: 2338
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on a diff in pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1289448738
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -127,10 +136,16 @@ BufferedLogChannel createNewLog(File dirForNextEntryLog) throws IOException {
BufferedLogChannel createNewLogForCompaction(File dirForNextEntryLog) throws IOException {
synchronized (createCompactionLogLock) {
- return allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ BufferedLogChannel bc = allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ writingCompactingLogId = bc.getLogId();
+ return bc;
}
}
+ void clearCompactingLogId() {
+ writingCompactingLogId = -1;
Review Comment:
Do we need to guard this by `synchronized (createCompactionLogLock)`? It seems not possible to have race conditions, but I see the `createNewLogForCompaction(File dirForNextEntryLog)` is guarded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Reidddddd commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1722890601
Hope to see this improvement in the next release (thumb up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1710419626
> Great job! Would you please add a test to protect this change? Especially for the sealed flag
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1478279917
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -64,10 +71,22 @@ public int read(ByteBuf dest, long pos) throws IOException {
return read(dest, pos, dest.writableBytes());
}
+ @Override
+ public long size() throws IOException {
+ if (sealed) {
+ if (fileSize == -1) {
+ fileSize = validateAndGetFileChannel().size();
+ }
+ return fileSize;
+ } else {
+ return validateAndGetFileChannel().size();
+ }
+ }
Review Comment:
@hangc0276 I was referring to https://github.com/apache/bookkeeper/blob/22097343ceed1312964ad077cdcd9fbd7e3e026b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java#L49-L52 in BufferedChannel class. In the case that the instance is a BufferedChannel class, perhaps it could use that information?
Is the challenge in that case that the same file could have a BufferedReadChannel instance and a BufferedChannel instance at the same time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1478009221
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -127,10 +136,16 @@ BufferedLogChannel createNewLog(File dirForNextEntryLog) throws IOException {
BufferedLogChannel createNewLogForCompaction(File dirForNextEntryLog) throws IOException {
synchronized (createCompactionLogLock) {
- return allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ BufferedLogChannel bc = allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ writingCompactingLogId = bc.getLogId();
+ return bc;
}
}
+ void clearCompactingLogId() {
+ writingCompactingLogId = -1;
Review Comment:
> No need this, the `createNewLogForCompaction` and `clearCompactingLogId` are serial.
What do you mean?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1652685967
Good idea, great results.
Can you please check that these "Bookie Tests" failures aren't related the change:
```
Error: Errors:
Error: org.apache.bookkeeper.bookie.DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval
Error: Run 1: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
Error: Run 2: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
Error: Run 3: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
[INFO]
Error: org.apache.bookkeeper.bookie.storage.ldb.DbLedgerStorageTest.testEntriesOutOfOrderWithFlush
Error: Run 1: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
Error: Run 2: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
Error: Run 3: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
[INFO]
Warning: Flakes:
Warning: InterleavedLedgerStorageTest.testConsistencyCheckConcurrentGC
[INFO] Run 1: PASS
Error: Run 2: expected:<0> but was:<8354>
Error: Run 3: expected:<0> but was:<7255>
Error: Run 4: expected:<0> but was:<7712>
```
I'll restart the job to see if these are flaps though I don't think these tests flapped before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1663184468
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1485622225
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -64,10 +71,22 @@ public int read(ByteBuf dest, long pos) throws IOException {
return read(dest, pos, dest.writableBytes());
}
+ @Override
+ public long size() throws IOException {
+ if (sealed) {
+ if (fileSize == -1) {
Review Comment:
nit: if the file is already "sealed", should we read the size in the constructor? This way we wouldn't have any concurrency issues here if 2 threads call `size()` at the size time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Reidddddd commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1661477820
+1 from me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1478007578
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -90,16 +92,19 @@ synchronized long getPreallocatedLogId() {
return preallocatedLogId;
}
+ public boolean isSealed(long logId) {
+ return logId != writingLogId && logId != writingCompactingLogId;
+ }
Review Comment:
Isn't this misleading if it is called "isSealed"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1278597731
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/EntryLogger.java:
##########
@@ -129,4 +129,6 @@ default EntryLogMetadata getEntryLogMetadata(long entryLogId) throws IOException
* @return false if the entrylog doesn't exist.
*/
boolean removeEntryLog(long entryLogId);
+
+ String name();
Review Comment:
Is this a 'name' or a 'type' ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1659986769
> > > And i skimmed the changes, seems `if (xxx instance of DefaultEntryLogger)` will resolve it? no need to add this method
> >
> >
> > Using instanceOf is not a good practice, It violates polymorphism.
>
> `compactingLogWriteDone` is only a private method in `TransactionalEntryLogCompactor.java`, not a common method in `AbstractLogCompactor`.
>
> What you did, in order to serve a private function in a concrete sub class, you added a function and parameters for other classes which don't need them
>
> When you are talking about polymorphism, polymorphism allows each sub-class has its own behaviour, you are enforcing all the sub-class to satisfy a concrete need not a common need, you are the one who breaks polymorphism...
Ok, that makes sense. I would like to public an interface to do the work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1707804146
From the user report, they cherry-pick this commit, and they found that there is a problem about it.
```
Caused by: org.apache.bookkeeper.bookie.DefaultEntryLogger$EntryLookupException$MissingEntryException: pos 1027337410 (entry 135 for ledgerId 574) past end of entryLog 46
```
I will try to find the problem with them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1478010207
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -127,10 +136,16 @@ BufferedLogChannel createNewLog(File dirForNextEntryLog) throws IOException {
BufferedLogChannel createNewLogForCompaction(File dirForNextEntryLog) throws IOException {
synchronized (createCompactionLogLock) {
- return allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ BufferedLogChannel bc = allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ writingCompactingLogId = bc.getLogId();
+ return bc;
}
}
+ void clearCompactingLogId() {
+ writingCompactingLogId = -1;
Review Comment:
Just wondering if there's a chance that writingLogId and writingCompactingLogId field changes wouldn't be visible for another thread that reads these fields.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1484912473
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -127,10 +136,16 @@ BufferedLogChannel createNewLog(File dirForNextEntryLog) throws IOException {
BufferedLogChannel createNewLogForCompaction(File dirForNextEntryLog) throws IOException {
synchronized (createCompactionLogLock) {
- return allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ BufferedLogChannel bc = allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ writingCompactingLogId = bc.getLogId();
+ return bc;
}
}
+ void clearCompactingLogId() {
+ writingCompactingLogId = -1;
Review Comment:
> Just wondering if there's a chance that writingLogId and writingCompactingLogId field changes wouldn't be visible for another thread that reads these fields.
Make sense, make it volatile.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -127,10 +136,16 @@ BufferedLogChannel createNewLog(File dirForNextEntryLog) throws IOException {
BufferedLogChannel createNewLogForCompaction(File dirForNextEntryLog) throws IOException {
synchronized (createCompactionLogLock) {
- return allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ BufferedLogChannel bc = allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ writingCompactingLogId = bc.getLogId();
+ return bc;
}
}
+ void clearCompactingLogId() {
+ writingCompactingLogId = -1;
Review Comment:
> > No need this, the `createNewLogForCompaction` and `clearCompactingLogId` are serial.
>
> What do you mean?
The method `createNewLogForCompaction` is synchronized. It creates compact file one by one, so the previous log file id can't override the next log file id.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -90,16 +92,19 @@ synchronized long getPreallocatedLogId() {
return preallocatedLogId;
}
+ public boolean isSealed(long logId) {
+ return logId != writingLogId && logId != writingCompactingLogId;
+ }
Review Comment:
`isSealed` means that the entry log file will no longer be written to, we needn't care that the file sile change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1288158840
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -43,9 +43,12 @@ public class BufferedReadChannel extends BufferedChannelBase {
long invocationCount = 0;
long cacheHitCount = 0;
+ private long fileSize = -1;
+ private final boolean sealed;
- public BufferedReadChannel(FileChannel fileChannel, int readCapacity) {
+ public BufferedReadChannel(FileChannel fileChannel, int readCapacity, boolean sealed) {
Review Comment:
Do we need to keep the original constructor method and create a new one with `sealed` flag? Because the `BufferedReadChannel` class is public and the constructor is also public, other applications may use this channel to create instances. If we change the constructor directly, it will break the compatibility with the old versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1652738334
> Good idea, great results.
>
> Can you please check that these "Bookie Tests" failures aren't related the change:
>
> ```
> Error: Errors:
> Error: org.apache.bookkeeper.bookie.DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval
> Error: Run 1: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
> Error: Run 2: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
> Error: Run 3: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
> [INFO]
> Error: org.apache.bookkeeper.bookie.storage.ldb.DbLedgerStorageTest.testEntriesOutOfOrderWithFlush
> Error: Run 1: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
> Error: Run 2: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
> Error: Run 3: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
> [INFO]
> Warning: Flakes:
> Warning: InterleavedLedgerStorageTest.testConsistencyCheckConcurrentGC
> [INFO] Run 1: PASS
> Error: Run 2: expected:<0> but was:<8354>
> Error: Run 3: expected:<0> but was:<7255>
> Error: Run 4: expected:<0> but was:<7712>
> ```
>
> I'll restart the job to see if these are flaps though I don't think these tests flapped before.
I have checked the testAppendLedgersMapOnCacheRemoval and testEntriesOutOfOrderWithFlush. The problem is that the BufferedReadChannel and BufferedLogChannel share the same FileChannel. When the BufferedReadChannel finishes reading the fileSize, the BufferedLogChannel writes new data into the file. I will verify if this situation exists. In theory, the FileChannel read by the BufferedLogChannel should not write any new data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Reidddddd commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1657424879
I feel Enrico's purpose is to require some method docs to explain why it needs the method `name` or `type` (anyway)
And i skimmed the changes, seems `if (xxx instance of DefaultEntryLogger)` will resolve it? no need to add this method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1660152794
> I feel Enrico's purpose is to require some method docs to explain why it needs the method `name` or `type` (anyway)
>
> And i skimmed the changes, seems `if (xxx instance of DefaultEntryLogger)` will resolve it? no need to add this method
Finally, take your suggestion, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1661425141
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1484921503
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -64,10 +71,22 @@ public int read(ByteBuf dest, long pos) throws IOException {
return read(dest, pos, dest.writableBytes());
}
+ @Override
+ public long size() throws IOException {
+ if (sealed) {
+ if (fileSize == -1) {
+ fileSize = validateAndGetFileChannel().size();
+ }
+ return fileSize;
+ } else {
+ return validateAndGetFileChannel().size();
+ }
+ }
Review Comment:
I'm not got you fully, the BufferedChannel means the corresponding file is writing, it override the `BufferedReadChannel#read` method, it know the file end position.
> Is the challenge in that case that the same file could have a BufferedReadChannel instance and a BufferedChannel instance at the same time?
https://github.com/apache/bookkeeper/pull/4038#issuecomment-1652685967
The test contains the case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1710419296
> From the user report, they cherry-pick this commit, and they found that there is a problem about it.
>
> ```
> Caused by: org.apache.bookkeeper.bookie.DefaultEntryLogger$EntryLookupException$MissingEntryException: pos 1027337410 (entry 135 for ledgerId 574) past end of entryLog 46
> ```
>
> I will try to find the problem with them.
The user config `setEntryLogPerLedgerEnabled=true`, so the defaultEntryLogger will write multi entry log file at the same time, so the sealed flag is not set correctly.
If the config `setEntryLogPerLedgerEnabled=true`, the sealed flag always be false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1653891157
> Good idea, great results.
>
> Can you please check that these "Bookie Tests" failures aren't related the change:
>
> ```
> Error: Errors:
> Error: org.apache.bookkeeper.bookie.DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval
> Error: Run 1: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
> Error: Run 2: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
> Error: Run 3: DefaultEntryLogTest.testAppendLedgersMapOnCacheRemoval:1223 » IO Not all ledge...
> [INFO]
> Error: org.apache.bookkeeper.bookie.storage.ldb.DbLedgerStorageTest.testEntriesOutOfOrderWithFlush
> Error: Run 1: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
> Error: Run 2: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
> Error: Run 3: DbLedgerStorageTest.testEntriesOutOfOrderWithFlush:400 » IO Bad entry read fro...
> [INFO]
> Warning: Flakes:
> Warning: InterleavedLedgerStorageTest.testConsistencyCheckConcurrentGC
> [INFO] Run 1: PASS
> Error: Run 2: expected:<0> but was:<8354>
> Error: Run 3: expected:<0> but was:<7255>
> Error: Run 4: expected:<0> but was:<7712>
> ```
>
> I'll restart the job to see if these are flaps though I don't think these tests flapped before.
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1659868013
> And i skimmed the changes, seems `if (xxx instance of DefaultEntryLogger)` will resolve it? no need to add this method
Using instanceOf is not a good practice, It violates polymorphism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1478011716
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -64,10 +71,22 @@ public int read(ByteBuf dest, long pos) throws IOException {
return read(dest, pos, dest.writableBytes());
}
+ @Override
+ public long size() throws IOException {
+ if (sealed) {
+ if (fileSize == -1) {
+ fileSize = validateAndGetFileChannel().size();
+ }
+ return fileSize;
+ } else {
+ return validateAndGetFileChannel().size();
+ }
+ }
Review Comment:
The FileChnanel doesn't have `position` field and the `position()` method will trigger the OS system call
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy closed pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy closed pull request #4038: Improve DefaultEntryLogger read performance.
URL: https://github.com/apache/bookkeeper/pull/4038
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Reidddddd commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "Reidddddd (via GitHub)" <gi...@apache.org>.
Reidddddd commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1659939963
> > And i skimmed the changes, seems `if (xxx instance of DefaultEntryLogger)` will resolve it? no need to add this method
>
> Using instanceOf is not a good practice, It violates polymorphism.
`compactingLogWriteDone` is only a private method in `TransactionalEntryLogCompactor.java`, not a common method in `AbstractLogCompactor`.
What you did, in order to serve a private function in a concrete sub class, you added a function and parameters for other classes which don't need them
When you are talking about polymorphism, polymorphism allows each sub-class has its own behaviour, you are enforcing all the sub-class to satisfy a concrete need not a common need, you are the one who breaks polymorphism...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1289456715
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -43,9 +43,12 @@ public class BufferedReadChannel extends BufferedChannelBase {
long invocationCount = 0;
long cacheHitCount = 0;
+ private long fileSize = -1;
+ private final boolean sealed;
- public BufferedReadChannel(FileChannel fileChannel, int readCapacity) {
+ public BufferedReadChannel(FileChannel fileChannel, int readCapacity, boolean sealed) {
Review Comment:
Make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1926295014
@merlimat @lhotari Please help take a look at this PR, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1485640048
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -64,10 +71,22 @@ public int read(ByteBuf dest, long pos) throws IOException {
return read(dest, pos, dest.writableBytes());
}
+ @Override
+ public long size() throws IOException {
+ if (sealed) {
+ if (fileSize == -1) {
Review Comment:
The `size()` would throw IOException, I don't want to make the constructor announce the exception, so make the fileSize double check to avoid concurrency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1478279917
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -64,10 +71,22 @@ public int read(ByteBuf dest, long pos) throws IOException {
return read(dest, pos, dest.writableBytes());
}
+ @Override
+ public long size() throws IOException {
+ if (sealed) {
+ if (fileSize == -1) {
+ fileSize = validateAndGetFileChannel().size();
+ }
+ return fileSize;
+ } else {
+ return validateAndGetFileChannel().size();
+ }
+ }
Review Comment:
I was referring to https://github.com/apache/bookkeeper/blob/22097343ceed1312964ad077cdcd9fbd7e3e026b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java#L49-L52 in BufferedChannel class. In the case that the instance is a BufferedChannel class, perhaps it could use that information?
Is the challenge in that case that the same file could have a BufferedReadChannel instance and a BufferedChannel instance at the same time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1484909743
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -90,16 +92,19 @@ synchronized long getPreallocatedLogId() {
return preallocatedLogId;
}
+ public boolean isSealed(long logId) {
+ return logId != writingLogId && logId != writingCompactingLogId;
+ }
Review Comment:
`isSealed` means that the entry log file will no longer be written to, we needn't care that the file size change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve DefaultEntryLogger read performance. [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1477999505
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java:
##########
@@ -64,10 +71,22 @@ public int read(ByteBuf dest, long pos) throws IOException {
return read(dest, pos, dest.writableBytes());
}
+ @Override
+ public long size() throws IOException {
+ if (sealed) {
+ if (fileSize == -1) {
+ fileSize = validateAndGetFileChannel().size();
+ }
+ return fileSize;
+ } else {
+ return validateAndGetFileChannel().size();
+ }
+ }
Review Comment:
Would there be a benefit to override this logic in `BufferedChannel` class where the `position` (and possibly `unpersistedBytes`) field information would be leveraged? Doesn't `position` contain the file size when it's writable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1289471828
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLoggerAllocator.java:
##########
@@ -127,10 +136,16 @@ BufferedLogChannel createNewLog(File dirForNextEntryLog) throws IOException {
BufferedLogChannel createNewLogForCompaction(File dirForNextEntryLog) throws IOException {
synchronized (createCompactionLogLock) {
- return allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ BufferedLogChannel bc = allocateNewLog(dirForNextEntryLog, COMPACTING_SUFFIX);
+ writingCompactingLogId = bc.getLogId();
+ return bc;
}
}
+ void clearCompactingLogId() {
+ writingCompactingLogId = -1;
Review Comment:
No need this, the `createNewLogForCompaction` and `clearCompactingLogId` are serial.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#discussion_r1278639379
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/EntryLogger.java:
##########
@@ -129,4 +129,6 @@ default EntryLogMetadata getEntryLogMetadata(long entryLogId) throws IOException
* @return false if the entrylog doesn't exist.
*/
boolean removeEntryLog(long entryLogId);
+
+ String name();
Review Comment:
make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1660293044
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on pull request #4038: Improve DefaultEntryLogger read performance.
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #4038:
URL: https://github.com/apache/bookkeeper/pull/4038#issuecomment-1660503760
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org