[GitHub] reddycharan commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

reddycharan commented on a change in pull request #1281: Issue #570: 
Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176638435
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -802,88 +864,207 @@ private long readLastLogId(File f) {
 }
 }
 
+interface EntryLogManager {
+/*
+ * acquire lock for this ledger.
+ */
+void acquireLock(Long ledgerId);
+
+/*
+ * acquire lock for this ledger if it is not already available for this
+ * ledger then it will create a new one and then acquire lock.
+ */
+void acquireLockByCreatingIfRequired(Long ledgerId);
+
+/*
+ * release lock for this ledger
+ */
+void releaseLock(Long ledgerId);
+
+/*
+ * sets the logChannel for the given ledgerId. The previous one will be
+ * removed from replicaOfCurrentLogChannels. Previous logChannel will 
be
+ * added to rotatedLogChannels.
+ */
+void setCurrentLogForLedger(Long ledgerId, BufferedLogChannel 
logChannel);
 
 Review comment:
   With my changes in this sub-task and subsequent sub-tasks I would like to 
abstract out the code/logic pertaining to singleentrylog/entrylogperledger and 
move it to the implementations of EntryLogManager, but leave the rest of the 
code as it is. With that reasoning, these methods in EntryLogManager and 
variables need to be exposed and EntryLogger class has to work with these 
methods.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1285: Fixed typos in README.md

[GitBox]

sijie commented on issue #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285#issuecomment-375530945
 
 
   merged. 
   
   @dmsergeevp44 thank you for your contribution!


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie closed pull request #1285: Fixed typos in README.md

[GitBox]

sijie closed pull request #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/README.md b/README.md
index 2e03ddf46..aeeac68c1 100644
--- a/README.md
+++ b/README.md
@@ -9,7 +9,7 @@
 
 Apache BookKeeper is a scalable, fault tolerant and low latency storage 
service optimized for append-only workloads.
 
-It is suitable for being used in following scenerios:
+It is suitable for being used in following scenarios:
 
 - WAL (Write-Ahead-Logging), e.g. HDFS NameNode.
 - Message Store, e.g. Apache Pulsar.
@@ -30,7 +30,7 @@ It is suitable for being used in following scenerios:
 * [Tutorial](http://bookkeeper.apache.org/docs/master/bookkeeperTutorial.html)
 * [Java API](http://bookkeeper.apache.org/docs/master/apidocs/)
 
-You can also read [Turning Ledgers into 
Logs](http://bookkeeper.apache.org/docs/master/bookkeeperLedgers2Logs.html) to 
learn how to turn **ledgers** into continous **log streams**.
+You can also read [Turning Ledgers into 
Logs](http://bookkeeper.apache.org/docs/master/bookkeeperLedgers2Logs.html) to 
learn how to turn **ledgers** into continuous **log streams**.
 If you are looking for a high level **log stream** API, you can checkout 
[DistributedLog](http://distributedlog.io).
 
 ### Administrators
@@ -52,7 +52,7 @@ For filing bugs, suggesting improvements, or requesting new 
features, help us ou
 
 [Subscribe](mailto:user-subscr...@bookkeeper.apache.org) or 
[mail](mailto:u...@bookkeeper.apache.org) the 
[u...@bookkeeper.apache.org](mailto:u...@bookkeeper.apache.org) list - Ask 
questions, find answers, and also help other users.
 
-[Subscribe](mailto:dev-subscr...@bookkeeper.apache.org) or 
[mail](mailto:d...@bookkeeper.apache.org) the 
[d...@bookkeeper.apache.org](mailto:d...@bookkeeper.apache.org) list - Join 
developement discussions, propose new ideas and connect with contributors.
+[Subscribe](mailto:dev-subscr...@bookkeeper.apache.org) or 
[mail](mailto:d...@bookkeeper.apache.org) the 
[d...@bookkeeper.apache.org](mailto:d...@bookkeeper.apache.org) list - Join 
development discussions, propose new ideas and connect with contributors.
 
 [Join us on Slack](https://apachebookkeeper.herokuapp.com/) - This is the most 
immediate way to connect with Apache BookKeeper committers and contributors.
 


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1285: Fixed typos in README.md

[GitBox]

sijie commented on issue #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285#issuecomment-375530739
 
 
   IGNORE CI


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #1287: NPE at DNS.reverse

[GitBox]

sijie opened a new issue #1287: NPE at DNS.reverse
URL: https://github.com/apache/bookkeeper/issues/1287
 
 
   
   
   **BUG REPORT**
   
   1. Please describe the issue you observed:
   
   - What did you do?
   
   run the test
   
   - What did you expect to see?
   
   tests should pass
   
   - What did you see instead?
   
   It fails at starting the bookie as NPE is thrown at `DNS.reverseDns`.
   
   ```
   java.lang.NullPointerException
at org.apache.bookkeeper.net.DNS.reverseDns(DNS.java:90)
at org.apache.bookkeeper.net.DNS.getHosts(DNS.java:225)
at org.apache.bookkeeper.net.DNS.getDefaultHost(DNS.java:324)
at org.apache.bookkeeper.net.DNS.getDefaultHost(DNS.java:340)
at org.apache.bookkeeper.bookie.Bookie.getBookieAddress(Bookie.java:539)
at 
org.apache.bookkeeper.bookie.ScanAndCompareGarbageCollector.(ScanAndCompareGarbageCollector.java:96)
at 
org.apache.bookkeeper.bookie.GarbageCollectorThread.(GarbageCollectorThread.java:192)
at 
org.apache.bookkeeper.bookie.InterleavedLedgerStorage.initialize(InterleavedLedgerStorage.java:107)
at 
org.apache.bookkeeper.bookie.SortedLedgerStorage.initialize(SortedLedgerStorage.java:66)
at org.apache.bookkeeper.bookie.Bookie.(Bookie.java:721)
at 
org.apache.bookkeeper.proto.BookieServer.newBookie(BookieServer.java:115)
at org.apache.bookkeeper.proto.BookieServer.(BookieServer.java:96)
at 
org.apache.bookkeeper.test.BookKeeperClusterTestCase.startBookie(BookKeeperClusterTestCase.java:612)
at 
org.apache.bookkeeper.test.BookKeeperClusterTestCase.startNewBookieAndReturnAddress(BookKeeperClusterTestCase.java:595)
at 
org.apache.bookkeeper.test.BookKeeperClusterTestCase.startNewBookie(BookKeeperClusterTestCase.java:587)
at 
org.apache.bookkeeper.test.BookKeeperClusterTestCase.startBKCluster(BookKeeperClusterTestCase.java:219)
at 
org.apache.bookkeeper.test.BookKeeperClusterTestCase.setUp(BookKeeperClusterTestCase.java:134)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
at 
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at 
org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
at 
org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.lang.Thread.run(Thread.java:748)
   ```
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1286: WIP - Implement directly ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder

[GitBox]

sijie commented on issue #1286: WIP - Implement directly 
ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder
URL: https://github.com/apache/bookkeeper/pull/1286#issuecomment-375477291
 
 
   change looks good to me.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1285: Fixed typos in README.md

[GitBox]

sijie commented on issue #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285#issuecomment-375474920
 
 
   @dmsergeevp44 
   
   haha, it is a command for Jenkins CI, not for you. sorry for confusion.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on issue #1286: WIP - Implement directly ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder

[GitBox]

eolivelli commented on issue #1286: WIP - Implement directly 
ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder
URL: https://github.com/apache/bookkeeper/pull/1286#issuecomment-375474825
 
 
   retest this please


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on issue #1286: WIP - Implement directly ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder

[GitBox]

eolivelli commented on issue #1286: WIP - Implement directly 
ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder
URL: https://github.com/apache/bookkeeper/pull/1286#issuecomment-375474232
 
 
   I will also change the decoder side once we agree on this approach


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #1286: WIP - Implement directly ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder

[GitBox]

eolivelli commented on a change in pull request #1286: WIP - Implement directly 
ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder
URL: https://github.com/apache/bookkeeper/pull/1286#discussion_r176586775
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
 ##
 @@ -452,20 +454,24 @@ protected void decode(ChannelHandlerContext ctx, Object 
msg, List out) t
 }
 
 @Override
-protected void encode(ChannelHandlerContext ctx, Object msg, 
List out)
-throws Exception {
+public void write(ChannelHandlerContext ctx, Object msg, 
ChannelPromise promise) throws Exception {
 if (LOG.isTraceEnabled()) {
 LOG.trace("Encode response {} to channel {}.", msg, 
ctx.channel());
 }
 if (msg instanceof BookkeeperProtocol.Response) {
-out.add(repV3.encode(msg, ctx.alloc()));
+ctx.write(repV3.encode(msg, ctx.alloc()), promise);
 } else if (msg instanceof BookieProtocol.Response) {
-out.add(repPreV3.encode(msg, ctx.alloc()));
+ctx.write(repPreV3.encode(msg, ctx.alloc()), promise);
 } else {
 LOG.error("Invalid response to encode to {}: {}", 
ctx.channel(), msg.getClass().getName());
-out.add(msg);
+ctx.write(msg, promise);
 }
 }
+
+@Override
+public void exceptionCaught(ChannelHandlerContext ctx, Throwable 
cause) throws Exception {
+LOG.error("Generic error while encoding response", cause);
+}
 
 Review comment:
   Will drop


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] merlimat commented on a change in pull request #1286: WIP - Implement directly ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder

[GitBox]

merlimat commented on a change in pull request #1286: WIP - Implement directly 
ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder
URL: https://github.com/apache/bookkeeper/pull/1286#discussion_r176585565
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
 ##
 @@ -452,20 +454,24 @@ protected void decode(ChannelHandlerContext ctx, Object 
msg, List out) t
 }
 
 @Override
-protected void encode(ChannelHandlerContext ctx, Object msg, 
List out)
-throws Exception {
+public void write(ChannelHandlerContext ctx, Object msg, 
ChannelPromise promise) throws Exception {
 if (LOG.isTraceEnabled()) {
 LOG.trace("Encode response {} to channel {}.", msg, 
ctx.channel());
 }
 if (msg instanceof BookkeeperProtocol.Response) {
-out.add(repV3.encode(msg, ctx.alloc()));
+ctx.write(repV3.encode(msg, ctx.alloc()), promise);
 } else if (msg instanceof BookieProtocol.Response) {
-out.add(repPreV3.encode(msg, ctx.alloc()));
+ctx.write(repPreV3.encode(msg, ctx.alloc()), promise);
 } else {
 LOG.error("Invalid response to encode to {}: {}", 
ctx.channel(), msg.getClass().getName());
-out.add(msg);
+ctx.write(msg, promise);
 }
 }
+
+@Override
+public void exceptionCaught(ChannelHandlerContext ctx, Throwable 
cause) throws Exception {
+LOG.error("Generic error while encoding response", cause);
+}
 
 Review comment:
   I think this exception handler will be invoked in addition to the one we 
have already on the main handler.
   Also, the exception here might not necessarely related to the encoder, but 
the `exceptionCaught()` is invoked for any exception that happens in the IO 
thread when serving this connection.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] merlimat commented on issue #1285: Fixed typos in README.md

[GitBox]

merlimat commented on issue #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285#issuecomment-375471495
 
 
   > @sijie how would I go about retesting this?
   
   No worries, that's a command for Jenkins CI to get retriggered :)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dmsergeevp44 commented on issue #1285: Fixed typos in README.md

[GitBox]

dmsergeevp44 commented on issue #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285#issuecomment-375470136
 
 
   @sijie how would I go about retesting this?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1285: Fixed typos in README.md

[GitBox]

sijie commented on issue #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285#issuecomment-375464762
 
 
   retest this please


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] reddycharan commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

reddycharan commented on a change in pull request #1281: Issue #570: 
Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176549929
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1441,4 +1658,40 @@ static long fileName2LogId(String fileName) {
 static String logId2HexString(long logId) {
 return Long.toHexString(logId);
 }
-}
+
+/**
+ * Datastructure which maintains the status of logchannels. When a
+ * logChannel is created entry of < entryLogId, false > will be made to 
this
+ * sortedmap and when logChannel is rotated and flushed then the entry is
+ * updated to < entryLogId, true > and all the lowest entries with
+ * < entryLogId, true > status will be removed from the sortedmap. So that 
way
+ * we could get least unflushed LogId.
+ *
+ */
+static class RecentEntryLogsStatus {
+private SortedMap entryLogsStatusMap;
 
 Review comment:
   will do


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] reddycharan commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

reddycharan commented on a change in pull request #1281: Issue #570: 
Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176549632
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -802,88 +864,207 @@ private long readLastLogId(File f) {
 }
 }
 
+interface EntryLogManager {
+/*
+ * acquire lock for this ledger.
+ */
+void acquireLock(Long ledgerId);
 
 Review comment:
   currently in all the places 'synchronized' is used, but for 
entrylogperledger we should have a lock for each ledger/entrylog. Hence these 
lock methods in EntryLogManager. The contract with this interface is only in 
addEntry call "acquireLockByCreatingIfRequired" (because this call would create 
a lock if it is not created yet and we need this in addEntry method since 
addEntry will be the first call related to that ledger in write path) and for 
all other methods in EntryLogger when they are dealing/writing to that 
particular ledger/entrylog they would call "acquireLock". I don't see why it 
needs to be hidden. EntryLogger needs to synchronize, instead of synchronizing 
on its intrinsic lock it delegates it to EntryLogManager, and EntryLogManager 
deals with inner details but all it needs is call to acquireLock and 
releaseLock with ledgerId.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1284: Improve FileInfoBackingCache

[GitBox]

sijie commented on issue #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#issuecomment-375420639
 
 
   > Could you give a sequence of events for how the other race occurs?
   
   I am using your diagram and using the existing logic
   
   ```
 | Read Fi CacheLoader   | Write Fi CacheLoader 
   |
 | == | 
== |
 | calls loadFileInfo | 
   |
 | - checks fileInfos under read lock | 
   |
 || 
   |
 || calls loadFileInfo  
   |
 || - checks fileInfos under read 
lock |
 || - create a file info and put 
into fileInfos under write lock |
 | - create another file  info and put it  |
|
 | into fileInfos under write lock| 
   |
   ```
   
   The Read Fi CacheLoader will overwrite the fileinfo created by Write Fi 
CacheLoader.
   
   
   > Has the locking shown to be a problem in benching?
   
   it is a giant write lock. so every thread is blocking waiting for file 
lookup (which is done by the file loader).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1284: Improve FileInfoBackingCache

[GitBox]

sijie commented on issue #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#issuecomment-375420639
 
 
   > Could you give a sequence of events for how the other race occurs?
   
   I am using your diagram and using the existing logic
   
   ```
 | Read Fi CacheLoader   | Write Fi CacheLoader 
   |
 | == | 
== |
 | calls loadFileInfo | 
   |
 | - checks fileInfos under read lock | 
   |
 || 
   |
 || calls loadFileInfo  
   |
 || - checks fileInfos under read 
lock |
 || - create a file info and put 
into fileInfos under write lock |
 | - create another file  info and put it  |
|
 | into fileInfos under write lock| 
   |
   ```
   
   
   > Has the locking shown to be a problem in benching?
   
   it is a giant write lock. so every thread is blocking waiting for file 
lookup (which is done by the file loader).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1284: Improve FileInfoBackingCache

[GitBox]

sijie commented on issue #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#issuecomment-375420639
 
 
   > Could you give a sequence of events for how the other race occurs?
   
   I am using your diagram and using the existing logic
   
   ```
 | Read Fi CacheLoader   | Write Fi CacheLoader 
   |
 | == | 
== |
 | calls loadFileInfo | 
   |
 | - checks fileInfos under read lock | 
   |
 || 
   |
 || calls loadFileInfo  
   |
 || - checks fileInfos under read 
lock |
 || - create a file info and put 
into fileInfos under write lock |
 | - create another file ||
 |info and put it into||
 |  fileInfos under write lock ||
   ```
   
   
   > Has the locking shown to be a problem in benching?
   
   it is a giant write lock. so every thread is blocking waiting for file 
lookup (which is done by the file loader).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1284: Improve FileInfoBackingCache

[GitBox]

sijie commented on issue #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#issuecomment-375420639
 
 
   > Could you give a sequence of events for how the other race occurs?
   
   I am using your diagram and using the existing logic
   
   ```
 | Read Fi CacheLoader   | Write Fi CacheLoader 
   |
 | == | 
== |
 | calls loadFileInfo | 
   |
 | - checks fileInfos under read lock | 
   |
 |||
 || calls loadFileInfo  
   |
 || - checks fileInfos under read 
lock |
 || - create a file info and put 
into fileInfos under write lock |
 | - create another file info and put it into fileInfos under write lock |  
  |
   ```
   
   
   > Has the locking shown to be a problem in benching?
   
   it is a giant write lock. so every thread is blocking waiting for file 
lookup (which is done by the file loader).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #1284: Improve FileInfoBackingCache

[GitBox]

sijie commented on issue #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#issuecomment-375420639
 
 
   > Could you give a sequence of events for how the other race occurs?
   
   I am using your diagram and using the existing logic
   
   ```
 | Read Fi CacheLoader   | Write Fi CacheLoader 
   |
 | == | 
== |
 | calls loadFileInfo | 
   |
 | - checks fileInfos under read lock | 
   |
 |||
 || calls loadFileInfo  
   |
 || - checks fileInfos under read 
lock |
 || - create a file info and put 
into fileInfos under write lock |
 | - create another file | |
 |info and put it into fileInfos under write lock | 
   |
   ```
   
   
   > Has the locking shown to be a problem in benching?
   
   it is a giant write lock. so every thread is blocking waiting for file 
lookup (which is done by the file loader).
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1284: Improve FileInfoBackingCache

[GitBox]

sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176535276
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
 ##
 @@ -52,25 +63,29 @@ CachedFileInfo loadFileInfo(long ledgerId, byte[] 
masterKey) throws IOException
 // have been able to get a reference to it here.
 // The caller of loadFileInfo owns the refence, and is
 // responsible for calling the corresponding #release().
-boolean retained = fi.tryRetain();
-assert(retained);
-return fi;
+return tryRetainFileInfo(fi);
 }
 } finally {
 lock.readLock().unlock();
 }
 
+File backingFile = fileLoader.load(ledgerId, masterKey != null);
+CachedFileInfo newFi = new CachedFileInfo(ledgerId, backingFile, 
masterKey);
 
 Review comment:
   I am not sure the race condition you describe exists.
   
   1), this method is only called by guava cache when loading the file info. 
guava cache guarantees [awaiting pending load for a given 
key](https://google.github.io/guava/releases/23.0/api/docs/com/google/common/cache/Cache.html#get-K-java.util.concurrent.Callable-).
 so thread a and thread b can not happen on one guava cache. **so one has to be 
loading from write fi cache, the other one has to be loading from read fi 
cache. **
   
   2), the file info loader is basically doing file lookup and construct the 
file info objects. it doesn't touch any persistence state (e.g. create file). 
so if the file exists at disk, the file info object created by thread a and 
thread b are identical. so there is no race condition. so the only concern is 
file doesn't exist on disk, ThreadA picks a directory while ThreadB picks 
another directory.
   
   now lets talk about "ThreadA picks one directory while ThreadB picks another 
directory". 
   
   in this case, both ThreadA and ThreadB can't find file on the disk, it means 
it is a first time creation. a read request can only attempt to load file info 
after a write request creates a file info.
   
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java#L61
   
   so this case won't actually happen.
   
   
   hope this clarify.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1284: Improve FileInfoBackingCache

[GitBox]

sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176520016
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
 ##
 @@ -92,8 +107,20 @@ private void releaseFileInfo(long ledgerId, CachedFileInfo 
fileInfo) {
 }
 
 void closeAllWithoutFlushing() throws IOException {
-for (Map.Entry entry : fileInfos.entrySet()) {
-entry.getValue().close(false);
+try {
 
 Review comment:
   I don't think we should change the signature to throw unchecked io 
exception. checked exception is well handled in the whole path, but not 
unchecked exception. 
   
   The logic here is just to getting around this interface doesn't throw 
checked exception, so I am able to throw checked exception. there is no really 
matter it is unchecked execution exception or unchecked io exception.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache

[GitBox]

ivankelly commented on a change in pull request #1284: Improve 
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176516627
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
 ##
 @@ -92,8 +107,20 @@ private void releaseFileInfo(long ledgerId, CachedFileInfo 
fileInfo) {
 }
 
 void closeAllWithoutFlushing() throws IOException {
-for (Map.Entry entry : fileInfos.entrySet()) {
-entry.getValue().close(false);
+try {
 
 Review comment:
   ya, saw that later. Better to use UncheckedIOException in any case. Maybe 
even change FileInfo close to throw it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on issue #1277: WIP - Use ByteBufOutputStream to serialize ProtoBuf messages

[GitBox]

eolivelli commented on issue #1277: WIP - Use ByteBufOutputStream to serialize 
ProtoBuf messages
URL: https://github.com/apache/bookkeeper/pull/1277#issuecomment-375366545
 
 
   Closing this PR in favour of #1286


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli closed pull request #1277: WIP - Use ByteBufOutputStream to serialize ProtoBuf messages

[GitBox]

eolivelli closed pull request #1277: WIP - Use ByteBufOutputStream to serialize 
ProtoBuf messages
URL: https://github.com/apache/bookkeeper/pull/1277
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
index a45f94eaa..02e5402d0 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtoEncoding.java
@@ -20,7 +20,6 @@
  */
 package org.apache.bookkeeper.proto;
 
-import com.google.protobuf.CodedOutputStream;
 import com.google.protobuf.ExtensionRegistry;
 import com.google.protobuf.InvalidProtocolBufferException;
 import com.google.protobuf.MessageLite;
@@ -356,14 +355,12 @@ private static ByteBuf serializeProtobuf(MessageLite msg, 
ByteBufAllocator alloc
 ByteBuf buf = allocator.heapBuffer(size, size);
 
 try {
-msg.writeTo(CodedOutputStream.newInstance(buf.array(), 
buf.arrayOffset() + buf.writerIndex(), size));
+msg.writeTo(new ByteBufOutputStream(buf));
 } catch (IOException e) {
 // This is in-memory serialization, should not fail
 throw new RuntimeException(e);
 }
 
-// Advance writer idx
-buf.writerIndex(buf.capacity());
 return buf;
 }
 


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli opened a new pull request #1286: WIP - Implement directly ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder

[GitBox]

eolivelli opened a new pull request #1286: WIP - Implement directly 
ChannelOutboundHandlerAdapter in BookieProtoEncoding#ResponseEncoder
URL: https://github.com/apache/bookkeeper/pull/1286
 
 
   This change prevents errors seen when Bookie runs on Java 9.
   
   In some cases response messages appears to be empty on the client side.
   There must be some race condition during serialization on messages when the 
bookie runs on Java 9, but there is way to create a simpler reproducer.
   Errors are reported only when the bookie is running with a real application
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache

[GitBox]

ivankelly commented on a change in pull request #1284: Improve 
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176476938
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
 ##
 @@ -52,25 +63,29 @@ CachedFileInfo loadFileInfo(long ledgerId, byte[] 
masterKey) throws IOException
 // have been able to get a reference to it here.
 // The caller of loadFileInfo owns the refence, and is
 // responsible for calling the corresponding #release().
-boolean retained = fi.tryRetain();
-assert(retained);
-return fi;
+return tryRetainFileInfo(fi);
 }
 } finally {
 lock.readLock().unlock();
 }
 
+File backingFile = fileLoader.load(ledgerId, masterKey != null);
+CachedFileInfo newFi = new CachedFileInfo(ledgerId, backingFile, 
masterKey);
 
 Review comment:
   I put the load inside the lock for a reason. This change introduces a race.
   
   ```
 | ThreadA| ThreadB 
   |
 | == | 
== |
 | calls loadInfoInfo | 
   |
 | - checks fileInfos under read lock | 
   |
 | - loads fileinfo for file  | 
   |
 || calls loadFileInfo  
   |
 || - checks fileInfos under read 
lock |
 || - loads fileinfo for file   
   |
 || - puts into fileInfos under 
write lock |
 || 
   |
 || // does something else, maybe 
fencing  |
 || 
   |
 || calls releaseFileInfo   
   |
 || - flushes out fencing flag  
   |
 || - removes from fileInfo 
   |
 | - puts info fileInfos under write lock | 
   |
   ```
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache

[GitBox]

ivankelly commented on a change in pull request #1284: Improve 
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176473048
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
 ##
 @@ -92,8 +107,20 @@ private void releaseFileInfo(long ledgerId, CachedFileInfo 
fileInfo) {
 }
 
 void closeAllWithoutFlushing() throws IOException {
-for (Map.Entry entry : fileInfos.entrySet()) {
-entry.getValue().close(false);
+try {
 
 Review comment:
   I don't see the point of this change. I'm pretty sure used a foreach loop in 
first place to avoid something like this.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] reddycharan commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

reddycharan commented on a change in pull request #1281: Issue #570: 
Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176460672
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -116,6 +121,18 @@ public ConcurrentLongLongHashMap getLedgersMap() {
 return entryLogMetadata.getLedgersMap();
 }
 
+public boolean isLedgerDirFull() {
 
 Review comment:
   ok will change that.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dmsergeevp44 opened a new pull request #1285: Fixed typos in README.md

[GitBox]

dmsergeevp44 opened a new pull request #1285: Fixed typos in README.md
URL: https://github.com/apache/bookkeeper/pull/1285
 
 
   Descriptions of the changes in this PR:
   
   (PR description content here)...
   
   Master Issue: #
   
   > ---
   > Be sure to do all of the following to help us incorporate your contribution
   > quickly and easily:
   >
   > If this PR is a BookKeeper Proposal (BP):
   >
   > - [ ] Make sure the PR title is formatted like:
   > `: Description of bookkeeper proposal`
   > `e.g. BP-1: 64 bits ledger is support`
   > - [ ] Attach the master issue link in the description of this PR.
   > - [ ] Attach the google doc link if the BP is written in Google Doc.
   >
   > Otherwise:
   > 
   > - [ ] Make sure the PR title is formatted like:
   > `: Description of pull request`
   > `e.g. Issue 123: Description ...`
   > - [ ] Make sure tests pass via `mvn clean apache-rat:check install 
spotbugs:check`.
   > - [ ] Replace `` in the title with the actual Issue number.
   > 
   > ---
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Jenkins build became unstable: bookkeeper_postcommit_master_java8 #78

[Apache Jenkins Server]

See 

Jenkins build is still unstable: bookkeeper_release_branch #96

[Apache Jenkins Server]

See 

Build failed in Jenkins: bookkeeper_postcommit_master_java9 #78

[Apache Jenkins Server]

See 


--
Started by timer
[EnvInject] - Loading node environment variables.
Building remotely on H23 (ubuntu xenial) in workspace 

 > git rev-parse --is-inside-work-tree # timeout=10
Fetching changes from the remote Git repository
 > git config remote.origin.url https://github.com/apache/bookkeeper.git # 
 > timeout=10
ERROR: Error fetching remote repo 'origin'
hudson.plugins.git.GitException: Failed to fetch from 
https://github.com/apache/bookkeeper.git
at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:862)
at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1129)
at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1160)
at hudson.scm.SCM.checkout(SCM.java:495)
at hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
at hudson.model.Run.execute(Run.java:1724)
at hudson.maven.MavenModuleSetBuild.run(MavenModuleSetBuild.java:543)
at hudson.model.ResourceController.execute(ResourceController.java:97)
at hudson.model.Executor.run(Executor.java:429)
Caused by: hudson.plugins.git.GitException: Command "git config 
remote.origin.url https://github.com/apache/bookkeeper.git; returned status 
code 4:
stdout: 
stderr: error: failed to write new configuration file 


at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1996)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1964)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1960)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1597)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1609)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.setRemoteUrl(CliGitAPIImpl.java:1243)
at hudson.plugins.git.GitAPI.setRemoteUrl(GitAPI.java:160)
at sun.reflect.GeneratedMethodAccessor236.invoke(Unknown Source)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
hudson.remoting.RemoteInvocationHandler$RPCRequest.perform(RemoteInvocationHandler.java:922)
at 
hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:896)
at 
hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:853)
at hudson.remoting.UserRequest.perform(UserRequest.java:207)
at hudson.remoting.UserRequest.perform(UserRequest.java:53)
at hudson.remoting.Request$2.run(Request.java:358)
at 
hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:72)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:748)
Suppressed: hudson.remoting.Channel$CallSiteStackTrace: Remote call to 
H23
at 
hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1693)
at hudson.remoting.UserResponse.retrieve(UserRequest.java:310)
at hudson.remoting.Channel.call(Channel.java:908)
at 
hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:281)
at com.sun.proxy.$Proxy110.setRemoteUrl(Unknown Source)
at 
org.jenkinsci.plugins.gitclient.RemoteGitImpl.setRemoteUrl(RemoteGitImpl.java:295)
at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:850)
at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1129)
at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1160)
at hudson.scm.SCM.checkout(SCM.java:495)
at 
hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
at 
jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
at hudson.model.Run.execute(Run.java:1724)
at 

Build failed in Jenkins: bookkeeper_release_nightly_snapshot #106

[Apache Jenkins Server]

See 


--
Started by timer
[EnvInject] - Loading node environment variables.
Building remotely on H23 (ubuntu xenial) in workspace 

 > git rev-parse --is-inside-work-tree # timeout=10
Fetching changes from the remote Git repository
 > git config remote.origin.url https://github.com/apache/bookkeeper.git # 
 > timeout=10
ERROR: Error fetching remote repo 'origin'
hudson.plugins.git.GitException: Failed to fetch from 
https://github.com/apache/bookkeeper.git
at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:862)
at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1129)
at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1160)
at hudson.scm.SCM.checkout(SCM.java:495)
at hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
at hudson.model.Run.execute(Run.java:1724)
at hudson.maven.MavenModuleSetBuild.run(MavenModuleSetBuild.java:543)
at hudson.model.ResourceController.execute(ResourceController.java:97)
at hudson.model.Executor.run(Executor.java:429)
Caused by: hudson.plugins.git.GitException: Command "git config 
remote.origin.url https://github.com/apache/bookkeeper.git; returned status 
code 4:
stdout: 
stderr: error: failed to write new configuration file 


at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1996)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1964)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1960)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1597)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1609)
at 
org.jenkinsci.plugins.gitclient.CliGitAPIImpl.setRemoteUrl(CliGitAPIImpl.java:1243)
at hudson.plugins.git.GitAPI.setRemoteUrl(GitAPI.java:160)
at sun.reflect.GeneratedMethodAccessor236.invoke(Unknown Source)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
hudson.remoting.RemoteInvocationHandler$RPCRequest.perform(RemoteInvocationHandler.java:922)
at 
hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:896)
at 
hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:853)
at hudson.remoting.UserRequest.perform(UserRequest.java:207)
at hudson.remoting.UserRequest.perform(UserRequest.java:53)
at hudson.remoting.Request$2.run(Request.java:358)
at 
hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:72)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:748)
Suppressed: hudson.remoting.Channel$CallSiteStackTrace: Remote call to 
H23
at 
hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1693)
at hudson.remoting.UserResponse.retrieve(UserRequest.java:310)
at hudson.remoting.Channel.call(Channel.java:908)
at 
hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:281)
at com.sun.proxy.$Proxy110.setRemoteUrl(Unknown Source)
at 
org.jenkinsci.plugins.gitclient.RemoteGitImpl.setRemoteUrl(RemoteGitImpl.java:295)
at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:850)
at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1129)
at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1160)
at hudson.scm.SCM.checkout(SCM.java:495)
at 
hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
at 
jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
at 
hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
at hudson.model.Run.execute(Run.java:1724)
at 

[GitHub] sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on a change in pull request #1281: Issue #570: Introducing 
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176336059
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -802,88 +864,207 @@ private long readLastLogId(File f) {
 }
 }
 
+interface EntryLogManager {
+/*
+ * acquire lock for this ledger.
+ */
+void acquireLock(Long ledgerId);
 
 Review comment:
   Can we try to hide these locking stuffs?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on a change in pull request #1281: Issue #570: Introducing 
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176336283
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -802,88 +864,207 @@ private long readLastLogId(File f) {
 }
 }
 
+interface EntryLogManager {
+/*
+ * acquire lock for this ledger.
+ */
+void acquireLock(Long ledgerId);
+
+/*
+ * acquire lock for this ledger if it is not already available for this
+ * ledger then it will create a new one and then acquire lock.
+ */
+void acquireLockByCreatingIfRequired(Long ledgerId);
+
+/*
+ * release lock for this ledger
+ */
+void releaseLock(Long ledgerId);
+
+/*
+ * sets the logChannel for the given ledgerId. The previous one will be
+ * removed from replicaOfCurrentLogChannels. Previous logChannel will 
be
+ * added to rotatedLogChannels.
+ */
+void setCurrentLogForLedger(Long ledgerId, BufferedLogChannel 
logChannel);
 
 Review comment:
   All the `currentLogForLedger` `copy of rotated log channels` `copy of 
current logs` are kind of implementation detail of an implementation of 
EntryLogManager. Can we think of a better abstraction to hide these?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on a change in pull request #1281: Issue #570: Introducing 
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176332343
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
 ##
 @@ -584,15 +584,6 @@ public void 
testWithDiskFullReadOnlyDisabledOrForceGCAllowDisabled() throws Exce
 } catch (NoWritableLedgerDirException e) {
 // expected
 }
-
 
 Review comment:
   Can you explain why these lines are removed?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on a change in pull request #1281: Issue #570: Introducing 
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176332723
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1441,4 +1658,40 @@ static long fileName2LogId(String fileName) {
 static String logId2HexString(long logId) {
 return Long.toHexString(logId);
 }
-}
+
+/**
+ * Datastructure which maintains the status of logchannels. When a
+ * logChannel is created entry of < entryLogId, false > will be made to 
this
+ * sortedmap and when logChannel is rotated and flushed then the entry is
+ * updated to < entryLogId, true > and all the lowest entries with
+ * < entryLogId, true > status will be removed from the sortedmap. So that 
way
+ * we could get least unflushed LogId.
+ *
+ */
+static class RecentEntryLogsStatus {
+private SortedMap entryLogsStatusMap;
 
 Review comment:
   "private final"


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on a change in pull request #1281: Issue #570: Introducing 
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176331968
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
 ##
 @@ -209,16 +202,12 @@ public void onSizeLimitReached(final Checkpoint cp) 
throws IOException {
 public void run() {
 try {
 LOG.info("Started flushing mem table.");
-long logIdBeforeFlush = entryLogger.getCurrentLogId();
 memTable.flush(SortedLedgerStorage.this);
-long logIdAfterFlush = entryLogger.getCurrentLogId();
 // in any case that an entry log reaches the limit, we 
roll the log and start checkpointing.
 // if a memory table is flushed spanning over two entry 
log files, we also roll log. this is
 // for performance consideration: since we don't wanna 
checkpoint a new log file that ledger
 // storage is writing to.
-if (entryLogger.reachEntryLogLimit(0) || logIdAfterFlush 
!= logIdBeforeFlush) {
-LOG.info("Rolling entry logger since it reached size 
limitation");
-entryLogger.rollLog();
+if (entryLogger.rollLogsIfEntryLogLimitReached()) {
 
 Review comment:
   This changes performance characteristics.  


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on a change in pull request #1281: Issue #570: Introducing 
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176331262
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -116,6 +121,18 @@ public ConcurrentLongLongHashMap getLedgersMap() {
 return entryLogMetadata.getLedgersMap();
 }
 
+public boolean isLedgerDirFull() {
 
 Review comment:
   it seems that you are converting this class to inner class just because you 
want to reference ledgerDirsManager. I don't think this is a good change. 
   
   I would prefer not reference `ledgerDirManager`. you should just add 
`getFile()` in this class and call 
`ledgerDirsManager.isDirFull(channel.getFile())`.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on a change in pull request #1281: Issue #570: Introducing 
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r176332233
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
 ##
 @@ -167,14 +167,7 @@ public ByteBuf getEntry(long ledgerId, long entryId) 
throws IOException {
 
 @Override
 public void checkpoint(final Checkpoint checkpoint) throws IOException {
-long numBytesFlushed = memTable.flush(this, checkpoint);
-if (numBytesFlushed > 0) {
-// if bytes are added between previous flush and this checkpoint,
-// it means bytes might live at current active entry log, we need
-// roll current entry log and then issue checkpoint to underlying
-// interleaved ledger storage.
-entryLogger.rollLog();
-}
+memTable.flush(this, checkpoint);
 
 Review comment:
   I would suggest leaving this change out of this PR. Let's discuss this when 
you introduce multiple entry log manager.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie closed pull request #1280: Improve write rejection in DbLedgerStorage

[GitBox]

sijie closed pull request #1280: Improve write rejection in DbLedgerStorage 
URL: https://github.com/apache/bookkeeper/pull/1280
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java
index 99ae39e76..b7428b282 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java
@@ -82,6 +82,7 @@ public static BookieException create(int code) {
 int CookieNotFoundException = -105;
 int MetadataStoreException = -106;
 int UnknownBookieIdException = -107;
+int OperationRejectedException = -108;
 }
 
 public void setCode(int code) {
@@ -122,6 +123,9 @@ public String getMessage(int code) {
 case Code.UnknownBookieIdException:
 err = "Unknown bookie id";
 break;
+case Code.OperationRejectedException:
+err = "Operation rejected";
+break;
 default:
 err = "Invalid operation";
 break;
@@ -174,6 +178,22 @@ public LedgerFencedException() {
 }
 }
 
+/**
+ * Signals that a ledger has been fenced in a bookie. No more entries can 
be appended to that ledger.
+ */
+public static class OperationRejectedException extends BookieException {
+public OperationRejectedException() {
+super(Code.OperationRejectedException);
+}
+
+@Override
+public Throwable fillInStackTrace() {
+// Since this exception is a way to signal a specific condition 
and it's triggered and very specific points,
+// we can disable stack traces.
+return null;
+}
+}
+
 /**
  * Signal that an invalid cookie is found when starting a bookie.
  *
diff --git 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java
index f4db7a689..23840be27 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java
@@ -73,7 +73,7 @@ static ByteBuf createLedgerFenceEntry(Long ledgerId) {
  */
 abstract SettableFuture fenceAndLogInJournal(Journal journal) 
throws IOException;
 
-abstract long addEntry(ByteBuf entry) throws IOException;
+abstract long addEntry(ByteBuf entry) throws IOException, BookieException;
 abstract ByteBuf readEntry(long entryId) throws IOException;
 
 abstract long getLastAddConfirmed() throws IOException;
diff --git 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java
index 84730cb76..a0f34eab6 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java
@@ -138,7 +138,7 @@ ByteBuf getExplicitLac() {
 }
 
 @Override
-long addEntry(ByteBuf entry) throws IOException {
+long addEntry(ByteBuf entry) throws IOException, BookieException {
 long ledgerId = entry.getLong(entry.readerIndex());
 
 if (ledgerId != this.ledgerId) {
diff --git 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java
index 83ac2c0ae..dcfac3147 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java
@@ -100,7 +100,7 @@ void initialize(ServerConfiguration conf,
  *
  * @return the entry id of the entry added
  */
-long addEntry(ByteBuf entry) throws IOException;
+long addEntry(ByteBuf entry) throws IOException, BookieException;
 
 /**
  * Read an entry from storage.
diff --git 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java
index ff9cd316a..55b094587 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java
@@ -42,13 +42,13 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;