[GitHub] sijie closed pull request #245: Update README status about distributedlog project

[GitBox]

sijie closed pull request #245: Update README status about distributedlog 
project
URL: https://github.com/apache/distributedlog/pull/245
 
 
   

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 9af2f356..77a14c0c 100644
--- a/README.md
+++ b/README.md
@@ -14,6 +14,9 @@ reliable _real-time_ applications.
 
 _Apache DistributedLog project graduated from Incubator at July 2017. It is 
now a sub-project of Apache BookKeeper._
 
+The core components of _Apache DistributedLog_ has been merged as part of 
_Apache BookKeeper_. The development of _Apache DistributedLog_ has been moved 
under BookKeeper.
+See [BP-26: Move distributedlog library as part of 
bookkeeper](http://bookkeeper.apache.org/bps/BP-26-move-distributedlog-core-library/)
 for more details.
+
 ## Features
 
  High Performance


 


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 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_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_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] 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] 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 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)

[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_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_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] 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-376417461
 
 
   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 closed pull request #1288: Add sync variants of all methods in handles

[GitBox]

eolivelli 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