[GitHub] sijie closed pull request #1290: Added executeOrdered to complement submitOrdered

[GitBox]

sijie closed pull request #1290: Added executeOrdered to complement 
submitOrdered
URL: https://github.com/apache/bookkeeper/pull/1290
 
 
   

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-common/src/main/java/org/apache/bookkeeper/common/util/OrderedScheduler.java
 
b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedScheduler.java
index b24fb95fa..bf2a6fbc8 100644
--- 
a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedScheduler.java
+++ 
b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedScheduler.java
@@ -329,6 +329,16 @@ public void submit(SafeRunnable r) {
 return chooseThread(orderingKey).submit(timedRunnable(r));
 }
 
+/**
+ * schedules a one time action to execute with an ordering guarantee on 
the orderingKey.
+ *
+ * @param orderingKey order key to submit the task
+ * @param r task to run
+ */
+public void executeOrdered(Object orderingKey, SafeRunnable r) {
+chooseThread(orderingKey).execute(timedRunnable(r));
+}
+
 /**
  * schedules a one time action to execute with an ordering guarantee on 
the key.
  *


 


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 #1290: Added executeOrdered to complement submitOrdered

[GitBox]

sijie commented on issue #1290: Added executeOrdered to complement submitOrdered
URL: https://github.com/apache/bookkeeper/pull/1290#issuecomment-376777075
 
 
   (the CI failure is unrelated)


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 #1290: Added executeOrdered to complement submitOrdered

[GitBox]

sijie commented on issue #1290: Added executeOrdered to complement submitOrdered
URL: https://github.com/apache/bookkeeper/pull/1290#issuecomment-376777028
 
 
   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 commented on issue #1294: BK configuration file updates

[GitBox]

sijie commented on issue #1294: BK configuration file updates
URL: https://github.com/apache/bookkeeper/pull/1294#issuecomment-376776487
 
 
   rebased to latest master


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 #1296: Create 4.6.2 documentation stub

[GitBox]

sijie commented on issue #1296: Create 4.6.2 documentation stub
URL: https://github.com/apache/bookkeeper/pull/1296#issuecomment-376776154
 
 
   (TRAVIS PASSED since travis the only CI validate building site. so it is 
okay to 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 commented on issue #1296: Create 4.6.2 documentation stub

[GitBox]

sijie commented on issue #1296: Create 4.6.2 documentation stub
URL: https://github.com/apache/bookkeeper/pull/1296#issuecomment-376776002
 
 
   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 closed pull request #1288: Add sync variants of all methods in handles

[GitBox]

sijie closed pull request #1288: Add sync variants of all methods in handles
URL: https://github.com/apache/bookkeeper/pull/1288
 
 
   

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/client/LedgerHandle.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
index eebe43ce9..8db566739 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
@@ -389,14 +389,14 @@ void writeLedgerConfig(GenericCallback writeCb) {
 @Override
 public void close()
 throws InterruptedException, BKException {
-SyncCallbackUtils.waitForResult(asyncClose());
+SyncCallbackUtils.waitForResult(closeAsync());
 }
 
 /**
  * {@inheritDoc}
  */
 @Override
-public CompletableFuture asyncClose() {
+public CompletableFuture closeAsync() {
 CompletableFuture result = new CompletableFuture<>();
 SyncCloseCallback callback = new SyncCloseCallback(result);
 asyncClose(callback, null);
@@ -707,7 +707,7 @@ public void asyncReadUnconfirmedEntries(long firstEntry, 
long lastEntry, ReadCal
  *  id of last entry of sequence
  */
 @Override
-public CompletableFuture read(long firstEntry, long 
lastEntry) {
+public CompletableFuture readAsync(long firstEntry, long 
lastEntry) {
 // Little sanity check
 if (firstEntry < 0 || firstEntry > lastEntry) {
 LOG.error("IncorrectParameterException on ledgerId:{} 
firstEntry:{} lastEntry:{}",
@@ -748,7 +748,7 @@ public void asyncReadUnconfirmedEntries(long firstEntry, 
long lastEntry, ReadCal
  * @see #readUnconfirmedEntries(long, long)
  */
 @Override
-public CompletableFuture readUnconfirmed(long firstEntry, 
long lastEntry) {
+public CompletableFuture readUnconfirmedAsync(long 
firstEntry, long lastEntry) {
 // Little sanity check
 if (firstEntry < 0 || firstEntry > lastEntry) {
 LOG.error("IncorrectParameterException on ledgerId:{} 
firstEntry:{} lastEntry:{}",
@@ -852,7 +852,7 @@ public long addEntry(byte[] data) throws 
InterruptedException, BKException {
  * {@inheritDoc}
  */
 @Override
-public CompletableFuture append(ByteBuf data) {
+public CompletableFuture appendAsync(ByteBuf data) {
 SyncAddCallback callback = new SyncAddCallback();
 asyncAddEntry(data, callback, null);
 return callback;
@@ -1206,7 +1206,7 @@ public void readLastConfirmedDataComplete(int rc, 
DigestManager.RecoveryData dat
  * {@inheritDoc}
  */
 @Override
-public CompletableFuture tryReadLastAddConfirmed() {
+public CompletableFuture tryReadLastAddConfirmedAsync() {
 FutureReadLastConfirmed result = new FutureReadLastConfirmed();
 asyncTryReadLastConfirmed(result, null);
 return result;
@@ -1216,7 +1216,7 @@ public void readLastConfirmedDataComplete(int rc, 
DigestManager.RecoveryData dat
  * {@inheritDoc}
  */
 @Override
-public CompletableFuture readLastAddConfirmed() {
+public CompletableFuture readLastAddConfirmedAsync() {
 FutureReadLastConfirmed result = new FutureReadLastConfirmed();
 asyncReadLastConfirmed(result, null);
 return result;
@@ -1226,9 +1226,9 @@ public void readLastConfirmedDataComplete(int rc, 
DigestManager.RecoveryData dat
  * {@inheritDoc}
  */
 @Override
-public CompletableFuture 
readLastAddConfirmedAndEntry(long entryId,
-   
  long timeOutInMillis,
-   
  boolean parallel) {
+public CompletableFuture 
readLastAddConfirmedAndEntryAsync(long entryId,
+   
   long timeOutInMillis,
+   
   boolean parallel) {
 FutureReadLastConfirmedAndEntry result = new 
FutureReadLastConfirmedAndEntry();
 asyncReadLastConfirmedAndEntry(entryId, timeOutInMillis, parallel, 
result, null);
 return result;
diff --git 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandleAdv.java
 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandleAdv.java
index 80581032b..afd56cf2e 100644
--- 
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandleAdv.java
+++ 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandleAdv.java
@@ -246,7 +246,7 @@ public Stri

[GitHub] sijie commented on issue #1299: [dlog] use Atomic***FieldUpdater and LongAdder if possible

[GitBox]

sijie commented on issue #1299: [dlog] use Atomic***FieldUpdater and LongAdder 
if possible
URL: https://github.com/apache/bookkeeper/pull/1299#issuecomment-376775102
 
 
   /cc @philipsu522 for reviews 


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 pull request #1300: [dlog] Fix TestZKLogStreamMetadataStore

[GitBox]

sijie opened a new pull request #1300: [dlog] Fix TestZKLogStreamMetadataStore
URL: https://github.com/apache/bookkeeper/pull/1300
 
 
   
   
   Descriptions of the changes in this PR:
   
   *Problem*
   
   All test cases in `TestZKLogStreamMetadataStore` share same zookeeper 
cluster. so if the tests
   run before `testGetMissingPathsRecursive` runs, they might create the 
missing path components
   for `testGetMissingPathsRecursive`, which will fail the assertion in 
`testGetMissingPathsRecursive`
   
   *Solution*
   
   Add unique suffix to "/path" for each test case to avoid conflicts
   
   
   > ---
   > 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

[GitHub] sijie commented on issue #1300: [dlog] Fix TestZKLogStreamMetadataStore

[GitBox]

sijie commented on issue #1300: [dlog] Fix TestZKLogStreamMetadataStore
URL: https://github.com/apache/bookkeeper/pull/1300#issuecomment-376775064
 
 
   /cc @philipsu522 for reviews


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 pull request #1299: [dlog] use Atomic***FieldUpdater and LongAdder if possible

[GitBox]

sijie opened a new pull request #1299: [dlog] use Atomic***FieldUpdater and 
LongAdder if possible
URL: https://github.com/apache/bookkeeper/pull/1299
 
 
   
   
   Descriptions of the changes in this PR:
   
   Atomic**FieldUpdater + volatile provides similar guarantees as Atomic*** but 
use much fewer memory.
   In dlog, when handling large number of streams, there can be a lot of 
Atomic*** fields with `LogHandler`, `SegmentWriter` and such.
   Switching using Atomic*** to using Atomic**FieldUpdater + volatile will save 
a lot of memory. See 
[details](http://normanmaurer.me/blog/2013/10/28/Lesser-known-concurrent-classes-Part-1/)
   
   LongAdder is less contended across threads, which is more preferable to 
AtomicLong when multiple threads update a common sum. E.g. 
SampledMovingAverageRate.
   Switching to use LongAdder if AtomicLong is not needed. See 
[details](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/LongAdder.html)
   
   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

[GitHub] reddycharan commented on issue #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

reddycharan commented on issue #1281: Issue #570: Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-376723194
 
 
   Thanks @ivankelly for taking the effort to consider ways to refactor the 
code. But this class is in deep down in the stack and I'm hesitant to make big 
code churn. 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 to have minimal code 
churn and more focus. Once things are in place, I can consider the refactoring 
aspect of these classes.


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_r177611374
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
 ##
 @@ -209,16 +211,39 @@ public void onSizeLimitReached(final Checkpoint cp) 
throws IOException {
 public void run() {
 try {
 LOG.info("Started flushing mem table.");
-long logIdBeforeFlush = entryLogger.getCurrentLogId();
+long prevAllocLogIdBeforeFlush = 
entryLogger.getPreviousAllocatedEntryLogId();
 
 Review comment:
   @reddycharan 
   
   >  why would you say that behavior would be different between preallocation 
enabled and disabled? Is it because of async nature of preallocation?
   
   yeah because preallocation is in background, preallocation can advance entry 
log id. if it does, the logic is not correct.
   
   > I can add "getCurrentLogId" to the EntryLogManager Interface. For 
EntryLogManagerForSingleEntryLog implementation it would give the logId of the 
current activelog (which is current behavior).
   
   That could work for now. Although I would still like to see if there is 
possibility to produce a cleaner interface without exposing those log ids. Let 
me think a bit more and come back.
   


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_r177590256
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
 ##
 @@ -209,16 +211,39 @@ public void onSizeLimitReached(final Checkpoint cp) 
throws IOException {
 public void run() {
 try {
 LOG.info("Started flushing mem table.");
-long logIdBeforeFlush = entryLogger.getCurrentLogId();
+long prevAllocLogIdBeforeFlush = 
entryLogger.getPreviousAllocatedEntryLogId();
 
 Review comment:
   "This change is not correct. Because it should be "current" log id, not 
"previous allocated entry log id". That says the behavior is different between 
preallocation enabled and disabled."
   
   @sijie why would you say that behavior would be different between 
preallocation enabled and disabled? Is it because of async nature of 
preallocation?
   
   Ok, I think I can do the following, which would solve 2 problems here.
   
   I can add "getCurrentLogId" to the EntryLogManager Interface. For 
EntryLogManagerForSingleEntryLog implementation it would give the logId of the 
current activelog (which is current behavior). For 
EntryLogManagerForEntryLogPerLedger it would give some constant value all the 
times and it should be fine in entryLogPerLedger case, because checkpoint is 
not made when entrylog is rotated but instead SyncThread drives it 
periodically. 
   
   Also it would remove the need of extra condition regarding 
"isTransactionalCompactionEnabled" which I added in this change.
   
   So it would be clean and straightforward.
   


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_r177532672
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
 ##
 @@ -209,16 +211,39 @@ public void onSizeLimitReached(final Checkpoint cp) 
throws IOException {
 public void run() {
 try {
 LOG.info("Started flushing mem table.");
-long logIdBeforeFlush = entryLogger.getCurrentLogId();
+long prevAllocLogIdBeforeFlush = 
entryLogger.getPreviousAllocatedEntryLogId();
 
 Review comment:
   This change is not correct. Because it should be "current" log id, not 
"previous allocated entry log id". That says the behavior is different between 
preallocation enabled and disabled.
   
   That leads me to think that current entry log manager interface is not 
abstracted with right methods. because it abstracts a lot of methods around the 
underlying files, which underlying files are kind of tight with corresponding 
checkpointing and flushing logic. two different entry log manager can't really 
fit in this interface or checkpointing logic as well. 
   
   So as what I said in my previous comment, we need to think about what is the 
right methods to put in EntryLogManager.
   
   > I would suggest the EntryLogManager should hide the details on how to 
create log, allocate log,
   > flush/checkpoint logs. the details such as preallocationLogId, 
unflushedLogId and how to lock them should be > hidden to the implementation, 
rather than defining a too finer interface here.
   


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_r177535531
 
 

 ##
 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:
   @ivankelly that's what I commented before. I don't think the methods in 
EntryLogManager provide the right abstraction. That's what I also dislike your 
approach on asking moving the class out at this PR. At this PR, we need to 
figure out what are the right methods to put in an EntryLogManager before any 
kind of code movement. You comments about inner classes complicate things.


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_r177532166
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
 ##
 @@ -78,6 +79,7 @@ public void initialize(ServerConfiguration conf,
 .setNameFormat("SortedLedgerStorage-%d")
 .setPriority((Thread.NORM_PRIORITY + Thread.MAX_PRIORITY) / 
2).build());
 this.stateManager = stateManager;
+this.isTransactionalCompactionEnabled = 
conf.getUseTransactionalCompaction();
 
 Review comment:
   @ivankelly 
   
   > The external behaviour of SortedLedgerStorage should not change with this 
patch
   
   it is not an external behavior of SortedLedgerStorage. It is the logic of 
SortedLedgerStorage. SortedLedgerStorage needs to realize the compaction 
mechanism to do checkpointing. That's related to the discussion below around 
checkpointing.
   


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 #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

sijie commented on issue #1281: Issue #570: Introducing EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-376631793
 
 
   @ivankelly : 
   
   appreciate you are working on this change. Here are my thought below:
   
   > If there are circular dependencies, then the abstraction is broken from 
the very start. I've commented on how to fix it, and pushed a sketched patch to 
master...ivankelly:entrylogmanager which deals with a lot of the issues.
   
   if `EntryLogManager` is an inner class (as what Charan) proposed, the 
abstraction is not *broken*. It is just need refactor later. I am saying 
"circular dependencies" is for what you said moving `EntryLogManager` out of 
the EntryLogger class. You need to move a lot of common code around to break 
the circular depenency, which I think that is too much for this PR or not worth 
at this moment. Because that is taking this PR into a whole "refactor" story, 
which I don't think we should take, because  1) we are losing the focus 2) it 
moves code around, make things a bit hard to review 3) the movement can be 
tricky, you don't really know if that works for multiple entry log.
   
   The approach what I am suggesting is focusing on the interface - what is the 
right methods that EntryLogManager need for both single entry log and multiple 
entry logs. Get the interface done, Get the multiple entry log implementation, 
and do the refactor to move entry log manager out.  That is the most practical 
approach than the reverse. Otherwise we might end up moving code around and 
eventually found that this movement doesn't work for multiple entry log 
approach.
   
   There is no real sense talking about inner class vs outer class before we 
get an interface abstraction that can work for both single entry log and 
multiple entry logs. That is my thought. but if you really feel strong about 
it, go for 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] sijie commented on issue #1283: Issue #1282: call to appendLedgersMap in flushCompactionLog

[GitBox]

sijie commented on issue #1283: Issue #1282: call to appendLedgersMap in 
flushCompactionLog
URL: https://github.com/apache/bookkeeper/pull/1283#issuecomment-376622093
 
 
   @reddycharan 
   
   >  I'm not sure what you meant by "What if we flush the log channel 
successfully but not the ledger map?". In which case can it happen? Are you 
saying that this scenario is not handled completely?
   
so @yzang was asking if the flush ordering at line 494
   ```
// flush the internal buffer back to filesystem but not sync 
disk
// so the readers could access the data from filesystem.
logChannel.flush();  logChannel.flush();
 
// Append ledgers map at the end of entry log
appendLedgersMap(logChannel);
   ```
   
   @yzang : the `flush` here is writing all the buffer data back to filesystem. 
it is *NOT* flushing to disk. so the order at line 494 is correct: we write the 
buffer back to filesystem and append the ledger map after that. then checkpoint 
logic will fsync the entry log file (via flushAndForceWrite) (which happens at 
flushCurrentLog and flushRotatedLogs).
   
   
   > Also shall we move line 494 to before line 841 instead? It makes more 
sense to append the ledger map in flush function than when creating a new log?
   
   @yzang 
   
   `flushRotatedLogs` only happens when checkpointing happen. The reason we put 
`appendLedgerMap()` at line 494 instead of before line 841 is a performance 
consideration. you can append ledger map into filesystem without fsync, so 
between you append ledger map and the real checkpoint happen, filesystem might 
already 'pdflush' the dirty pages back to disk. when the real fsync happen, 
there is no blocking I/O happening.
   
   @reddycharan @yzang hope this clarify the questions.
   
   
   
   
   
   


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 #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177515383
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockReadHandle.java
 ##
 @@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.client;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.client.api.LastConfirmedAndEntry;
+import org.apache.bookkeeper.client.api.LedgerEntries;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.apache.bookkeeper.client.api.LedgerMetadata;
+import org.apache.bookkeeper.client.api.ReadHandle;
+import org.apache.bookkeeper.client.impl.LedgerEntriesImpl;
+import org.apache.bookkeeper.client.impl.LedgerEntryImpl;
+
+
+/**
+ * Mock implementation of ReadHandle.
+ */
+@Slf4j
+public class MockReadHandle implements ReadHandle {
+private final MockBookKeeper bk;
+private final long ledgerId;
+private final LedgerMetadata metadata;
+private final List entries;
+
+MockReadHandle(MockBookKeeper bk, long ledgerId, LedgerMetadata metadata, 
List entries) {
+this.bk = bk;
+this.ledgerId = ledgerId;
+this.metadata = metadata;
+this.entries = entries;
+}
+
+@Override
+public CompletableFuture readAsync(long firstEntry, long 
lastEntry) {
+CompletableFuture promise = new CompletableFuture<>();
+if (bk.isStopped()) {
+promise.completeExceptionally(BKException.create(-1));
+return promise;
+}
+
+bk.executor.execute(() -> {
+if (bk.getProgrammedFailStatus()) {
+
promise.completeExceptionally(BKException.create(bk.failReturnCode));
+return;
+} else if (bk.isStopped()) {
+promise.completeExceptionally(BKException.create(-1));
+return;
+}
+
+log.debug("readEntries: first={} last={} total={}", 
firstEntry, lastEntry, entries.size());
+List seq = new ArrayList<>();
+long entryId = firstEntry;
+while (entryId <= lastEntry && entryId < entries.size()) {
+seq.add(entries.get((int) entryId++).duplicate());
+}
+log.debug("Entries read: {}", seq);
+promise.complete(LedgerEntriesImpl.create(seq));
+});
+return promise;
+
+}
+
+@Override
+public CompletableFuture readUnconfirmedAsync(long 
firstEntry, long lastEntry) {
+return readAsync(firstEntry, lastEntry);
+}
+
+@Override
+public CompletableFuture readLastAddConfirmedAsync() {
+return CompletableFuture.completedFuture((long) (entries.size() - 1));
 
 Review comment:
   `return CompletableFuture.completedFuture(getLastAddConfirmed())`


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 #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177515542
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockReadHandle.java
 ##
 @@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.client;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.client.api.LastConfirmedAndEntry;
+import org.apache.bookkeeper.client.api.LedgerEntries;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.apache.bookkeeper.client.api.LedgerMetadata;
+import org.apache.bookkeeper.client.api.ReadHandle;
+import org.apache.bookkeeper.client.impl.LedgerEntriesImpl;
+import org.apache.bookkeeper.client.impl.LedgerEntryImpl;
+
+
+/**
+ * Mock implementation of ReadHandle.
+ */
+@Slf4j
+public class MockReadHandle implements ReadHandle {
+private final MockBookKeeper bk;
+private final long ledgerId;
+private final LedgerMetadata metadata;
+private final List entries;
+
+MockReadHandle(MockBookKeeper bk, long ledgerId, LedgerMetadata metadata, 
List entries) {
+this.bk = bk;
+this.ledgerId = ledgerId;
+this.metadata = metadata;
+this.entries = entries;
+}
+
+@Override
+public CompletableFuture readAsync(long firstEntry, long 
lastEntry) {
+CompletableFuture promise = new CompletableFuture<>();
+if (bk.isStopped()) {
+promise.completeExceptionally(BKException.create(-1));
+return promise;
+}
+
+bk.executor.execute(() -> {
+if (bk.getProgrammedFailStatus()) {
+
promise.completeExceptionally(BKException.create(bk.failReturnCode));
+return;
+} else if (bk.isStopped()) {
+promise.completeExceptionally(BKException.create(-1));
+return;
+}
+
+log.debug("readEntries: first={} last={} total={}", 
firstEntry, lastEntry, entries.size());
+List seq = new ArrayList<>();
+long entryId = firstEntry;
+while (entryId <= lastEntry && entryId < entries.size()) {
+seq.add(entries.get((int) entryId++).duplicate());
+}
+log.debug("Entries read: {}", seq);
+promise.complete(LedgerEntriesImpl.create(seq));
+});
+return promise;
+
+}
+
+@Override
+public CompletableFuture readUnconfirmedAsync(long 
firstEntry, long lastEntry) {
+return readAsync(firstEntry, lastEntry);
+}
+
+@Override
+public CompletableFuture readLastAddConfirmedAsync() {
+return CompletableFuture.completedFuture((long) (entries.size() - 1));
+}
+
+@Override
+public CompletableFuture tryReadLastAddConfirmedAsync() {
+return readLastAddConfirmedAsync();
+}
+
+@Override
+public long getLastAddConfirmed() {
+return entries.size() - 1;
 
 Review comment:
   return the entry id of last entry


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 #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177514356
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockReadHandle.java
 ##
 @@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.client;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.client.api.LastConfirmedAndEntry;
+import org.apache.bookkeeper.client.api.LedgerEntries;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.apache.bookkeeper.client.api.LedgerMetadata;
+import org.apache.bookkeeper.client.api.ReadHandle;
+import org.apache.bookkeeper.client.impl.LedgerEntriesImpl;
+import org.apache.bookkeeper.client.impl.LedgerEntryImpl;
+
+
+/**
+ * Mock implementation of ReadHandle.
+ */
+@Slf4j
+public class MockReadHandle implements ReadHandle {
+private final MockBookKeeper bk;
+private final long ledgerId;
+private final LedgerMetadata metadata;
+private final List entries;
+
+MockReadHandle(MockBookKeeper bk, long ledgerId, LedgerMetadata metadata, 
List entries) {
+this.bk = bk;
+this.ledgerId = ledgerId;
+this.metadata = metadata;
+this.entries = entries;
+}
+
+@Override
+public CompletableFuture readAsync(long firstEntry, long 
lastEntry) {
+CompletableFuture promise = new CompletableFuture<>();
+if (bk.isStopped()) {
+promise.completeExceptionally(BKException.create(-1));
 
 Review comment:
   use `Code. ReadException` for -1.


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 #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177509903
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeper.java
 ##
 @@ -210,6 +213,66 @@ public void close() throws InterruptedException, 
BKException {
 shutdown();
 }
 
+@Override
+public OpenBuilder newOpenLedgerOp() {
+return new OpenBuilder() {
+long ledgerId;
+byte[] password;
+org.apache.bookkeeper.client.api.DigestType digestType;
 
 Review comment:
   why not import 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 a change in pull request #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177510570
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeper.java
 ##
 @@ -210,6 +213,66 @@ public void close() throws InterruptedException, 
BKException {
 shutdown();
 }
 
+@Override
+public OpenBuilder newOpenLedgerOp() {
+return new OpenBuilder() {
 
 Review comment:
   why not extending `OpenBuilderImpl` because most of the methods here are 
builder methods. what you really need is overriding the `execute` method.


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 #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177513577
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockLedgerHandle.java
 ##
 @@ -100,7 +108,7 @@ public void run() {
 final Queue seq = new ArrayDeque();
 long entryId = firstEntry;
 while (entryId <= lastEntry && entryId < entries.size()) {
-seq.add(entries.get((int) entryId++));
+seq.add(new LedgerEntry(entries.get((int) 
entryId++).duplicate()));
 
 Review comment:
   there is a memory leak here, you don't need duplicate.
   
   old `LedgerEntry` constructor will retain the buffer.


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 #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177516314
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockReadHandle.java
 ##
 @@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.client;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.client.api.LastConfirmedAndEntry;
+import org.apache.bookkeeper.client.api.LedgerEntries;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.apache.bookkeeper.client.api.LedgerMetadata;
+import org.apache.bookkeeper.client.api.ReadHandle;
+import org.apache.bookkeeper.client.impl.LedgerEntriesImpl;
+import org.apache.bookkeeper.client.impl.LedgerEntryImpl;
+
+
+/**
+ * Mock implementation of ReadHandle.
+ */
+@Slf4j
+public class MockReadHandle implements ReadHandle {
+private final MockBookKeeper bk;
+private final long ledgerId;
+private final LedgerMetadata metadata;
+private final List entries;
+
+MockReadHandle(MockBookKeeper bk, long ledgerId, LedgerMetadata metadata, 
List entries) {
+this.bk = bk;
+this.ledgerId = ledgerId;
+this.metadata = metadata;
+this.entries = entries;
+}
+
+@Override
+public CompletableFuture readAsync(long firstEntry, long 
lastEntry) {
+CompletableFuture promise = new CompletableFuture<>();
+if (bk.isStopped()) {
+promise.completeExceptionally(BKException.create(-1));
+return promise;
+}
+
+bk.executor.execute(() -> {
+if (bk.getProgrammedFailStatus()) {
+
promise.completeExceptionally(BKException.create(bk.failReturnCode));
+return;
+} else if (bk.isStopped()) {
+promise.completeExceptionally(BKException.create(-1));
+return;
+}
+
+log.debug("readEntries: first={} last={} total={}", 
firstEntry, lastEntry, entries.size());
+List seq = new ArrayList<>();
+long entryId = firstEntry;
+while (entryId <= lastEntry && entryId < entries.size()) {
+seq.add(entries.get((int) entryId++).duplicate());
+}
+log.debug("Entries read: {}", seq);
+promise.complete(LedgerEntriesImpl.create(seq));
+});
+return promise;
+
+}
+
+@Override
+public CompletableFuture readUnconfirmedAsync(long 
firstEntry, long lastEntry) {
+return readAsync(firstEntry, lastEntry);
+}
+
+@Override
+public CompletableFuture readLastAddConfirmedAsync() {
+return CompletableFuture.completedFuture((long) (entries.size() - 1));
+}
+
+@Override
+public CompletableFuture tryReadLastAddConfirmedAsync() {
+return readLastAddConfirmedAsync();
+}
+
+@Override
+public long getLastAddConfirmed() {
+return entries.size() - 1;
+}
+
+@Override
+public long getLength() {
+long length = 0;
+for (LedgerEntryImpl entry : entries) {
+length += entry.getLength();
+}
+
+return length;
+}
+
+@Override
+public boolean isClosed() {
+return metadata.isClosed();
+}
+
+@Override
+public CompletableFuture 
readLastAddConfirmedAndEntryAsync(long entryId,
+   
   long timeOutInMillis,
+   
   boolean parallel) {
+CompletableFuture promise = new 
CompletableFuture<>();
+// it doesn't make sense to implement long poll until WriteHandle is 
also implemented
 
 Review comment:
   the comment is misleading. long poll doesn't require write handle to 
implement. it is irrelative whether you use old write api or new write handle. 
please improve the comment or remove 

[GitHub] sijie commented on a change in pull request #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

sijie commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177513995
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockReadHandle.java
 ##
 @@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.client;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.client.api.LastConfirmedAndEntry;
+import org.apache.bookkeeper.client.api.LedgerEntries;
+import org.apache.bookkeeper.client.api.LedgerEntry;
+import org.apache.bookkeeper.client.api.LedgerMetadata;
+import org.apache.bookkeeper.client.api.ReadHandle;
+import org.apache.bookkeeper.client.impl.LedgerEntriesImpl;
+import org.apache.bookkeeper.client.impl.LedgerEntryImpl;
+
+
+/**
+ * Mock implementation of ReadHandle.
+ */
+@Slf4j
+public class MockReadHandle implements ReadHandle {
 
 Review comment:
   remove "public" to make it package protected.


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 closed issue #1287: NPE at DNS.reverse

[GitBox]

ivankelly closed issue #1287: NPE at DNS.reverse
URL: https://github.com/apache/bookkeeper/issues/1287
 
 
   


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 closed pull request #1295: Issue #1287: NPE at DNS.reverse

[GitBox]

ivankelly closed pull request #1295: Issue #1287: NPE at DNS.reverse
URL: https://github.com/apache/bookkeeper/pull/1295
 
 
   

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/net/DNS.java 
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/DNS.java
index 6ae5a8177..308728e7b 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/DNS.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/DNS.java
@@ -28,6 +28,7 @@
 import java.util.Vector;
 
 import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
 import javax.naming.directory.Attributes;
 import javax.naming.directory.DirContext;
 import javax.naming.directory.InitialDirContext;
@@ -87,7 +88,20 @@ public static String reverseDns(InetAddress hostIp, String 
ns)
 ictx.close();
 }
 
-return attribute.get("PTR").get().toString();
+if (null == attribute) {
+throw new NamingException("No attribute is found");
+}
+
+Attribute ptrAttr = attribute.get("PTR");
+if (null == ptrAttr) {
+throw new NamingException("No PTR attribute is found");
+}
+
+if (null == ptrAttr.get()) {
+throw new NamingException("PTR attribute value is null");
+}
+
+return ptrAttr.get().toString();
 }
 
 /**


 


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 issue #1295: Issue #1287: NPE at DNS.reverse

[GitBox]

ivankelly commented on issue #1295: Issue #1287: NPE at DNS.reverse
URL: https://github.com/apache/bookkeeper/pull/1295#issuecomment-376608384
 
 
   Just hit this in a CI run. Merging.


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 issue #1288: Add sync variants of all methods in handles

[GitBox]

ivankelly commented on issue #1288: Add sync variants of all methods in handles
URL: https://github.com/apache/bookkeeper/pull/1288#issuecomment-376594014
 
 
   @eolivelli there was a integration test failure, so kicking again. I think 
it's unrelated, but want to be sure. Also, too late in the day to dig deeper :)


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 issue #1288: Add sync variants of all methods in handles

[GitBox]

ivankelly commented on issue #1288: Add sync variants of all methods in handles
URL: https://github.com/apache/bookkeeper/pull/1288#issuecomment-376592561
 
 
   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] ivankelly commented on a change in pull request #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

ivankelly commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177471484
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeper.java
 ##
 @@ -210,6 +213,66 @@ public void close() throws InterruptedException, 
BKException {
 shutdown();
 }
 
+@Override
+public OpenBuilder newOpenLedgerOp() {
+return new OpenBuilder() {
+long ledgerId;
+byte[] password;
+org.apache.bookkeeper.client.api.DigestType digestType;
+
+@Override
+public OpenBuilder withLedgerId(long ledgerId) {
+this.ledgerId = ledgerId;
+return this;
+}
+
+@Override
+public OpenBuilder withRecovery(boolean recovery) {
+return this;
 
 Review comment:
   if needed. currently the mock calls asyncOpenLedger for asyncOpenNoRecovery. 
   
   Originally this code comes from pulsar, where they don't use tailing anyhow.


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 #1288: Add sync variants of all methods in handles

[GitBox]

eolivelli commented on issue #1288: Add sync variants of all methods in handles
URL: https://github.com/apache/bookkeeper/pull/1288#issuecomment-376570571
 
 
   I did not want to close this one


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 opened a new pull request #1288: Add sync variants of all methods in handles

[GitBox]

ivankelly opened a new pull request #1288: Add sync variants of all methods in 
handles
URL: https://github.com/apache/bookkeeper/pull/1288
 
 
   As discussed on the mailing list [1], this patch removes the
   inconsistency around the naming of the close call on the new handle
   APIs, by creating sync versions of each async calls, and renaming the
   async versions to have the suffix "Async".
   
   Most of the changes are very mechanical - just a copy of the old
   method and some small fixups the javadoc. One thing to note is that
   I've made a copy of the close and closeAsync methods in the
   WriteHandle interface, so that the ReadHandle and Handle javadoc for
   these methods do not have to talk about what it means to close/seal a
   ledger.
   
   Another change is that I've removed the SneakyThrows from close, that
   would have also been needed on the other sync methods. Instead, I pass a
   exception handler to FutureUtils which generates a BKException.
   
   [1] 
https://lists.apache.org/thread.html/c3784cffb949438510d21e5eac8c0351865c6748c42c380e673a60db@%3Cdev.bookkeeper.apache.org%3E
   


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 #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

eolivelli commented on a change in pull request #1298: newOpenLedgerOp and 
ReadHandle implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298#discussion_r177470458
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeper.java
 ##
 @@ -210,6 +213,66 @@ public void close() throws InterruptedException, 
BKException {
 shutdown();
 }
 
+@Override
+public OpenBuilder newOpenLedgerOp() {
+return new OpenBuilder() {
+long ledgerId;
+byte[] password;
+org.apache.bookkeeper.client.api.DigestType digestType;
+
+@Override
+public OpenBuilder withLedgerId(long ledgerId) {
+this.ledgerId = ledgerId;
+return this;
+}
+
+@Override
+public OpenBuilder withRecovery(boolean recovery) {
+return this;
 
 Review comment:
   I guess this will be enhanced when we will implement a new mock WriteHandle


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 is back to stable : bookkeeper_postcommit_master_java9 #83

[Apache Jenkins Server]

See 

Jenkins build is still unstable: bookkeeper_release_branch #101

[Apache Jenkins Server]

See 

[GitHub] ivankelly opened a new pull request #1298: newOpenLedgerOp and ReadHandle implementation for MockBookKeeper

[GitBox]

ivankelly opened a new pull request #1298: newOpenLedgerOp and ReadHandle 
implementation for MockBookKeeper
URL: https://github.com/apache/bookkeeper/pull/1298
 
 
   There are two implementations of the ReadHandle interface. One pure,
   which isn't backed by any production code, and another on
   MockLedgerHandle which passes through to the pure implementation.
   


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

Build failed in Jenkins: bookkeeper_postcommit_master_java8 #83

[Apache Jenkins Server]

See 


--
[...truncated 74.94 KB...]
2018-03-27T12:12:34.945 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven/2.0.6/maven-2.0.6.pom
2018-03-27T12:12:34.958 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven/2.0.6/maven-2.0.6.pom
 (9.0 kB at 696 kB/s)
2018-03-27T12:12:34.961 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-parent/5/maven-parent-5.pom
2018-03-27T12:12:34.975 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-parent/5/maven-parent-5.pom
 (15 kB at 1.0 MB/s)
2018-03-27T12:12:34.979 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-utils/1.4.1/plexus-utils-1.4.1.pom
2018-03-27T12:12:34.991 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-utils/1.4.1/plexus-utils-1.4.1.pom
 (1.9 kB at 159 kB/s)
2018-03-27T12:12:35.000 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-model/2.0.6/maven-model-2.0.6.pom
2018-03-27T12:12:35.012 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-model/2.0.6/maven-model-2.0.6.pom
 (3.0 kB at 234 kB/s)
2018-03-27T12:12:35.016 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-core/2.0.6/maven-core-2.0.6.pom
2018-03-27T12:12:35.028 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-core/2.0.6/maven-core-2.0.6.pom
 (6.7 kB at 559 kB/s)
2018-03-27T12:12:35.033 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-settings/2.0.6/maven-settings-2.0.6.pom
2018-03-27T12:12:35.044 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-settings/2.0.6/maven-settings-2.0.6.pom
 (2.0 kB at 182 kB/s)
2018-03-27T12:12:35.053 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-plugin-parameter-documenter/2.0.6/maven-plugin-parameter-documenter-2.0.6.pom
2018-03-27T12:12:35.067 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-plugin-parameter-documenter/2.0.6/maven-plugin-parameter-documenter-2.0.6.pom
 (1.9 kB at 136 kB/s)
2018-03-27T12:12:35.070 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/reporting/maven-reporting-api/2.0.6/maven-reporting-api-2.0.6.pom
2018-03-27T12:12:35.081 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/reporting/maven-reporting-api/2.0.6/maven-reporting-api-2.0.6.pom
 (1.8 kB at 159 kB/s)
2018-03-27T12:12:35.083 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/reporting/maven-reporting/2.0.6/maven-reporting-2.0.6.pom
2018-03-27T12:12:35.095 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/reporting/maven-reporting/2.0.6/maven-reporting-2.0.6.pom
 (1.4 kB at 120 kB/s)
2018-03-27T12:12:35.099 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/doxia/doxia-sink-api/1.0-alpha-7/doxia-sink-api-1.0-alpha-7.pom
2018-03-27T12:12:35.111 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/doxia/doxia-sink-api/1.0-alpha-7/doxia-sink-api-1.0-alpha-7.pom
 (424 B at 35 kB/s)
2018-03-27T12:12:35.118 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/doxia/doxia/1.0-alpha-7/doxia-1.0-alpha-7.pom
2018-03-27T12:12:35.131 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/doxia/doxia/1.0-alpha-7/doxia-1.0-alpha-7.pom
 (3.9 kB at 301 kB/s)
2018-03-27T12:12:35.134 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-profile/2.0.6/maven-profile-2.0.6.pom
2018-03-27T12:12:35.147 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-profile/2.0.6/maven-profile-2.0.6.pom
 (2.0 kB at 152 kB/s)
2018-03-27T12:12:35.151 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-repository-metadata/2.0.6/maven-repository-metadata-2.0.6.pom
2018-03-27T12:12:35.166 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-repository-metadata/2.0.6/maven-repository-metadata-2.0.6.pom
 (1.9 kB at 123 kB/s)
2018-03-27T12:12:35.170 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-error-diagnostics/2.0.6/maven-error-diagnostics-2.0.6.pom
2018-03-27T12:12:35.185 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-error-diagnostics/2.0.6/maven-error-diagnostics-2.0.6.pom
 (1.7 kB at 114 kB/s)
2018-03-27T12:12:35.193 [INFO] Downloading: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-project/2.0.6/maven-project-2.0.6.pom
2018-03-27T12:12:35.205 [INFO] Downloaded: 
https://repo.maven.apache.org/maven2/org/apache/maven/maven-project/2.0.6/maven-project-2.0.6.pom
 (2.6 kB at 220 kB/s)
2018-03-27T12:12

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

[GitBox]

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

 ##
 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:
   @sijie if ```rollLogsIfEntryLogLimitReached``` is false, it will fall 
through to the else if statement, which covers the flush over two logs.
   
   This is weird though, because it is really tied to a single log, but it acts 
like it's working with many.


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 #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

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

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -292,12 +304,37 @@ public EntryLogger(ServerConfiguration conf,
 logId = lastLogId;
 }
 }
-this.leastUnflushedLogId = logId + 1;
+this.recentlyCreatedEntryLogsStatus = new RecentEntryLogsStatus(logId 
+ 1);
 
 Review comment:
   EntryLogManager shouldn't call out to EntryLogger (see comment above). Both 
new log creation and flushing act almost entirely on the entrylogmanager. New 
log creation needs the listeners, but this is the only place the listeners are 
used, so they should be part of the entrylogmanager. EntryLogManager can have a 
reference to the RecentEntryLogStatus to update on flush.


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 #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

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

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -264,6 +274,8 @@ public EntryLogger(ServerConfiguration conf,
 // but the protocol varies so an exact value is difficult to determine
 this.maxSaneEntrySize = conf.getNettyMaxFrameSizeBytes() - 500;
 this.ledgerDirsManager = ledgerDirsManager;
+this.conf = conf;
+entryLogPerLedgerEnabled = conf.isEntryLogPerLedgerEnabled();
 
 Review comment:
   The current class hierarchy here looks like this:
   
![current](https://user-images.githubusercontent.com/54955/37962613-fbc5bb1a-31bb-11e8-9900-d0b68ecd6c64.jpg)
   
   There's no abstraction here. Everything is a cycle. It's not hard to break 
these up though. The coupling is fairly weak, and breaking it now will stop it 
from becoming strong in the future.
   
   Even just making EntryLoggerAllocator and the EntryLogManager static will 
take it a long way. With them static the code looks like 
https://github.com/ivankelly/bookkeeper/commit/62a333c768d7b7c0fa16e209470c82fed77eb594
   
   And the dependencies look like:
   
![static](https://user-images.githubusercontent.com/54955/37962791-7fb5dec8-31bc-11e8-9f83-9c3846fc177b.jpg)
   
   Still not ideal because there's a cycle between the allocator and the 
manager. This only exists so that createNewLog can select a directory. But 
almost all of the time, createNewLog is called from the manager (except for a 
compaction log, but that should go through manager also). So since that is the 
case, the manager can select the directory before the call and pass it in, 
breaking the cycle. See the code at 
https://github.com/ivankelly/bookkeeper/commit/2e29373570da59ff0c90f8983ee991c2826bf729
   
   The dependencies look like:
   
![nocycle](https://user-images.githubusercontent.com/54955/37962944-114aeb1c-31bd-11e8-9c61-0ef0d1e294c2.jpg)
   
   It's now possible to unit test each of these classes individually.
   
   
   


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 #1281: Issue #570: Introducing EntryLogManager.

[GitBox]

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

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
 ##
 @@ -78,6 +79,7 @@ public void initialize(ServerConfiguration conf,
 .setNameFormat("SortedLedgerStorage-%d")
 .setPriority((Thread.NORM_PRIORITY + Thread.MAX_PRIORITY) / 
2).build());
 this.stateManager = stateManager;
+this.isTransactionalCompactionEnabled = 
conf.getUseTransactionalCompaction();
 
 Review comment:
   I don't see what the point of this flag even is. The external behaviour of 
SortedLedgerStorage should not change with this patch, so why the flag?


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 #1297: Release notes for 4.6.2 version

[GitBox]

eolivelli opened a new pull request #1297: Release notes for 4.6.2 version
URL: https://github.com/apache/bookkeeper/pull/1297
 
 
   


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 #1296: Create 4.6.2 documentation stub

[GitBox]

eolivelli opened a new pull request #1296: Create 4.6.2 documentation stub
URL: https://github.com/apache/bookkeeper/pull/1296
 
 
   This change prepares the directory structure for docs of 4.6.2. New pages 
are not linked from menu.


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 #1268: (WIP) Enable bytebuf leak detection in tests

[GitBox]

eolivelli commented on issue #1268: (WIP) Enable bytebuf leak detection in tests
URL: https://github.com/apache/bookkeeper/pull/1268#issuecomment-376421725
 
 
   @sijie how is going this work?


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 pull request #1295: Issue #1287: NPE at DNS.reverse

[GitBox]

sijie opened a new pull request #1295: Issue #1287: NPE at DNS.reverse
URL: https://github.com/apache/bookkeeper/pull/1295
 
 
   
   
   Descriptions of the changes in this PR:
   
   *Problem*
   
   Null value can be returned on retrieving attributes.
   
   *Solution*
   
   If null value is returned, throw NamingException so cached localhost name is 
used.
   
   Master Issue: #1287 
   
   > ---
   > 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