[GitHub] sijie commented on issue #1308: longPollThreadPool can not be null
sijie commented on issue #1308: longPollThreadPool can not be null URL: https://github.com/apache/bookkeeper/pull/1308#issuecomment-377455751 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] sijie commented on a change in pull request #1311: (WIP) Issue #1310: Filter out body and masterKey from ProcessorV3s
sijie commented on a change in pull request #1311: (WIP) Issue #1310: Filter
out body and masterKey from ProcessorV3s
URL: https://github.com/apache/bookkeeper/pull/1311#discussion_r178238763
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessorV3.java
##
@@ -170,4 +170,18 @@ public void safeRun() {
requestProcessor.addRequestStats);
}
}
+
+/**
+ * this toString method filters out body and masterKey from the output.
+ * masterKey contains the password of the ledger and body is customer data,
+ * so it is not appropriate to have these in logs or system output.
+ */
+@Override
+public String toString() {
+Request.Builder reqBuilder =
Request.newBuilder().setHeader(request.getHeader());
Review comment:
rather than constructing another request without masterkey. why not
formatting the string itself (which it would void building another object)?
e.g.
```
@Override
public String toString() {
return MoreObjects.toStringHelper()
.add("ledgerId", request.getLedgerId())
...
.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] eolivelli commented on issue #80: BOOKKEEPER-816: use native fallocate to improve journal allocation
eolivelli commented on issue #80: BOOKKEEPER-816: use native fallocate to improve journal allocation URL: https://github.com/apache/bookkeeper/pull/80#issuecomment-377454214 Okay to me. Let's move forward. Let me try this final version on my work laptop 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 #1311: (WIP) Issue #1310: Filter out body and masterKey from ProcessorV3s
eolivelli commented on issue #1311: (WIP) Issue #1310: Filter out body and masterKey from ProcessorV3s URL: https://github.com/apache/bookkeeper/pull/1311#issuecomment-377453864 Sorry, just re read the issue. It is fine to me, the only issue I am thinking is that it is quite expensive 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 #1311: (WIP) Issue #1310: Filter out body and masterKey from ProcessorV3s
reddycharan commented on issue #1311: (WIP) Issue #1310: Filter out body and masterKey from ProcessorV3s URL: https://github.com/apache/bookkeeper/pull/1311#issuecomment-377450303 @sijie @merlimat before proceeding with other ProcessorV3s and testcases, I just wanted you to review this WIP PR. 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] philipsu522 commented on issue #1308: longPollThreadPool can not be null
philipsu522 commented on issue #1308: longPollThreadPool can not be null URL: https://github.com/apache/bookkeeper/pull/1308#issuecomment-377446799 shipit! 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 opened a new pull request #1311: (WIP) Issue #1310: Filter out body and masterKey from ProcessorV3s
reddycharan opened a new pull request #1311: (WIP) Issue #1310: Filter out body and masterKey from ProcessorV3s URL: https://github.com/apache/bookkeeper/pull/1311 Descriptions of the changes in this PR: In certain logs instances of ProcessorV3 (for eg WriteEntryProcessorV3) are logged. Logging entry data and credentials details is not appropriate. Filter out those details (have to consider other ProcessorV3s as well and testcases to validate this change) Master Issue: #1310 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 opened a new issue #1310: filter out confidential data (entry body and masterkey) from ProcessorV3 toString representation
reddycharan opened a new issue #1310: filter out confidential data (entry body and masterkey) from ProcessorV3 toString representation URL: https://github.com/apache/bookkeeper/issues/1310 **FEATURE REQUEST** 1. Please describe the feature you are requesting. In certain logs instances of ProcessorV3 (for eg WriteEntryProcessorV3) are logged. For eg - https://github.com/apache/bookkeeper/blob/master/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedScheduler.java#L172 This would print output of toString method of WriteEntryProcessorV3, which gets its output from request.toString (https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBaseV3.java#L94). request.toString would include all the fields of request like masterKey and body. For us 'body' is the customer data and masterKey contains ledger password details. So logging entry data and credentials details is not appropriate. 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] jvrao commented on issue #1309: Refactored OrderedSafeExecutor and OrderedScheduler
jvrao commented on issue #1309: Refactored OrderedSafeExecutor and OrderedScheduler URL: https://github.com/apache/bookkeeper/pull/1309#issuecomment-377435264 @merlimat As this is perf focused change, any data / micro benchmarks to validate our assumptions? 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] jvrao commented on issue #1309: Refactored OrderedSafeExecutor and OrderedScheduler
jvrao commented on issue #1309: Refactored OrderedSafeExecutor and OrderedScheduler URL: https://github.com/apache/bookkeeper/pull/1309#issuecomment-377418338 @dlg99 Please review. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on issue #1306: Minimize number of thread local instances in CRC32CDigestManager
merlimat commented on issue #1306: Minimize number of thread local instances in CRC32CDigestManager URL: https://github.com/apache/bookkeeper/pull/1306#issuecomment-377413752 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] merlimat commented on issue #1306: Minimize number of thread local instances in CRC32CDigestManager
merlimat commented on issue #1306: Minimize number of thread local instances in CRC32CDigestManager URL: https://github.com/apache/bookkeeper/pull/1306#issuecomment-377413752 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] merlimat commented on issue #1303: Make CRC32C_HASH a final static variable and log when we cannot use native crc32c library
merlimat commented on issue #1303: Make CRC32C_HASH a final static variable and log when we cannot use native crc32c library URL: https://github.com/apache/bookkeeper/pull/1303#issuecomment-377413704 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] merlimat commented on a change in pull request #1309: Refactored OrderedSafeExecutor and OrderedScheduler
merlimat commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178204624
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java
##
@@ -0,0 +1,529 @@
+/**
+ * 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.common.util;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.stats.Gauge;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.stats.OpStatsLogger;
+import org.apache.bookkeeper.stats.StatsLogger;
+import org.apache.commons.lang.StringUtils;
+
+/**
+ * This class provides 2 things over the java {@link ExecutorService}.
+ *
+ * 1. It takes {@link SafeRunnable objects} instead of plain Runnable
objects.
+ * This means that exceptions in scheduled tasks wont go unnoticed and will be
+ * logged.
+ *
+ * 2. It supports submitting tasks with an ordering key, so that tasks
submitted
+ * with the same key will always be executed in order, but tasks across
+ * different keys can be unordered. This retains parallelism while retaining
the
+ * basic amount of ordering we want (e.g. , per ledger handle). Ordering is
+ * achieved by hashing the key objects to threads by their {@link #hashCode()}
+ * method.
+ */
+@Slf4j
+public class OrderedExecutor implements ExecutorService {
+public static final int NO_TASK_LIMIT = -1;
+protected static final long WARN_TIME_MICRO_SEC_DEFAULT =
TimeUnit.SECONDS.toMicros(1);
+
+final String name;
+final ExecutorService threads[];
+final long threadIds[];
+final Random rand = new Random();
+final OpStatsLogger taskExecutionStats;
+final OpStatsLogger taskPendingStats;
+final boolean traceTaskExecution;
+final long warnTimeMicroSec;
+final int maxTasksInQueue;
+
+
+public static Builder newBuilder() {
+return new Builder();
+}
+
+/**
+ * A builder class for an OrderedSafeExecutor.
+ */
+public static class Builder extends AbstractBuilder {
+
+@Override
+public OrderedExecutor build() {
+if (null == threadFactory) {
+threadFactory = new
DefaultThreadFactory("bookkeeper-ordered-safe-executor");
+}
+return new OrderedExecutor(name, numThreads, threadFactory,
statsLogger,
+ traceTaskExecution,
warnTimeMicroSec, maxTasksInQueue);
+}
+}
+
+/**
+ * Builder to build ordered scheduler.
+ */
+public static class SchedulerBuilder extends
AbstractBuilder {}
+
+/**
+ * Abstract builder class to build {@link OrderedScheduler}.
+ */
+public abstract static class AbstractBuilder {
Review comment:
Actually, it needs to be public because the builder methods are returning
`AbstractBuilder` so that needs to be visible to callers.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat closed pull request #1292: Avoid acquiring closeLock.readLock() on every add/read operation
merlimat closed pull request #1292: Avoid acquiring closeLock.readLock() on
every add/read operation
URL: https://github.com/apache/bookkeeper/pull/1292
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java
index 0bd9fe8e1..285a06aa7 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClient.java
@@ -21,6 +21,7 @@
package org.apache.bookkeeper.proto;
import static com.google.common.base.Charsets.UTF_8;
+import static org.apache.bookkeeper.util.SafeRunnable.safeRun;
import com.google.common.collect.Lists;
import com.google.protobuf.ExtensionRegistry;
@@ -48,6 +49,7 @@
import org.apache.bookkeeper.auth.ClientAuthProvider;
import org.apache.bookkeeper.client.BKException;
import org.apache.bookkeeper.client.BookieInfoReader.BookieInfo;
+import org.apache.bookkeeper.common.util.SafeRunnable;
import org.apache.bookkeeper.conf.ClientConfiguration;
import org.apache.bookkeeper.net.BookieSocketAddress;
import org.apache.bookkeeper.proto.BookkeeperInternalCallbacks.GenericCallback;
@@ -62,7 +64,6 @@
import org.apache.bookkeeper.tls.SecurityHandlerFactory;
import org.apache.bookkeeper.util.ByteBufList;
import org.apache.bookkeeper.util.OrderedSafeExecutor;
-import org.apache.bookkeeper.util.SafeRunnable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -113,12 +114,9 @@ public BookieClient(ClientConfiguration conf,
EventLoopGroup eventLoopGroup,
this.scheduler = scheduler;
if (conf.getAddEntryTimeout() > 0 || conf.getReadEntryTimeout() > 0) {
-SafeRunnable monitor = new SafeRunnable() {
-@Override
-public void safeRun() {
-monitorPendingOperations();
-}
-};
+SafeRunnable monitor = safeRun(() -> {
+monitorPendingOperations();
+});
this.timeoutFuture = this.scheduler.scheduleAtFixedRate(monitor,
conf.getTimeoutMonitorIntervalSec(),
conf.getTimeoutMonitorIntervalSec(),
@@ -189,40 +187,29 @@ public PerChannelBookieClientPool
lookupClient(BookieSocketAddress addr) {
public void writeLac(final BookieSocketAddress addr, final long ledgerId,
final byte[] masterKey,
final long lac, final ByteBufList toSend, final WriteLacCallback
cb, final Object ctx) {
-closeLock.readLock().lock();
-try {
-final PerChannelBookieClientPool client = lookupClient(addr);
-if (client == null) {
-
cb.writeLacComplete(getRc(BKException.Code.BookieHandleNotAvailableException),
- ledgerId, addr, ctx);
-return;
-}
-
-toSend.retain();
-client.obtain(new GenericCallback() {
-@Override
-public void operationComplete(final int rc,
PerChannelBookieClient pcbc) {
-if (rc != BKException.Code.OK) {
-try {
-executor.submitOrdered(ledgerId, new
SafeRunnable() {
-@Override
-public void safeRun() {
-cb.writeLacComplete(rc, ledgerId, addr,
ctx);
-}
-});
-} catch (RejectedExecutionException re) {
-
cb.writeLacComplete(getRc(BKException.Code.InterruptedException), ledgerId,
addr, ctx);
-}
-} else {
-pcbc.writeLac(ledgerId, masterKey, lac, toSend, cb,
ctx);
-}
+final PerChannelBookieClientPool client = lookupClient(addr);
+if (client == null) {
+
cb.writeLacComplete(getRc(BKException.Code.BookieHandleNotAvailableException),
+ ledgerId, addr, ctx);
+return;
+}
-toSend.release();
+toSend.retain();
+client.obtain((rc, pcbc) -> {
+if (rc != BKException.Code.OK) {
+try {
+executor.submitOrdered(ledgerId, safeRun(() -> {
+cb.writeLacComplete(rc, ledgerId, addr, ctx);
+}));
+} catch (RejectedExecutionException re) {
+
cb.w
[GitHub] sijie commented on issue #1281: Issue #570: Introducing EntryLogManager.
sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377391260 > @sijie if you are ok with my comment regarding currentLogId, I can send next iteration change. go for it now. I will think of how to clean it up later. 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 #80: BOOKKEEPER-816: use native fallocate to improve journal allocation
sijie commented on issue #80: BOOKKEEPER-816: use native fallocate to improve journal allocation URL: https://github.com/apache/bookkeeper/pull/80#issuecomment-377390285 > We will need some follow up implementations in order to bundle native libs together with the jars on maven central. we will ship native library with binary distribution. it is a twitter portback to me, currently I don't have any plan for building it along to jars at this moment. 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 #80: BOOKKEEPER-816: use native fallocate to improve journal allocation
sijie commented on a change in pull request #80: BOOKKEEPER-816: use native
fallocate to improve journal allocation
URL: https://github.com/apache/bookkeeper/pull/80#discussion_r178196860
##
File path: bookkeeper-server/pom.xml
##
@@ -206,9 +206,96 @@
org.apache.maven.plugins
maven-checkstyle-plugin
+
+org.apache.maven.plugins
+maven-surefire-plugin
+${maven-surefire-plugin.version}
+
+ -Xmx2G -Djava.net.preferIPv4Stack=true
-Djava.library.path=${project.build.directory}/native/target/usr/local/lib
+ true
+ false
+ 1800
+ 2
+
+
+
+ native
+
+
+ native
+
+
+
+
+
+org.apache.maven.plugins
+maven-enforcer-plugin
+
+
+enforce-os
+
+ enforce
+
+
+
+
+ mac
+ unix
+ native build only supported on Mac or
Unix
+
+
+ true
+
+
+
+
+
+org.codehaus.mojo
+native-maven-plugin
+
+
+compile
+
+ javah
Review comment:
yes javac -h will do that. but jdk10 is out of the scope. again this is a
portback, it works for java8 and java9.
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 #80: BOOKKEEPER-816: use native fallocate to improve journal allocation
sijie commented on a change in pull request #80: BOOKKEEPER-816: use native
fallocate to improve journal allocation
URL: https://github.com/apache/bookkeeper/pull/80#discussion_r178196704
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java
##
@@ -797,6 +805,8 @@ public void scanJournal(long journalId, long journalPos,
JournalScanner scanner)
if (!isPaddingRecord) {
scanner.process(journalVersion, offset, recBuff);
}
+// update last log mark during replaying
+lastLogMark.setCurLogMark(journalId, offset);
}
Review comment:
it is already listed in the description. it is a portback, but if you feel
strong about it, I will break it down.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1309: Refactored OrderedSafeExecutor and OrderedScheduler
merlimat commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178190518
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/BoundedExecutorService.java
##
@@ -0,0 +1,109 @@
+/**
+ * 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.common.util;
+
+import com.google.common.util.concurrent.ForwardingExecutorService;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Implements {@link ExecutorService} and allows limiting the number of tasks
to
+ * be scheduled in the thread's queue.
+ */
+public class BoundedExecutorService extends ForwardingExecutorService {
+private final BlockingQueue queue;
+private final ThreadPoolExecutor thread;
+private final int maxTasksInQueue;
+
+public BoundedExecutorService(ThreadPoolExecutor thread, int
maxTasksInQueue) {
+this.queue = thread.getQueue();
+this.thread = thread;
+this.maxTasksInQueue = maxTasksInQueue;
+}
+
+@Override
+protected ExecutorService delegate() {
+return this.thread;
+}
+
+private void checkQueue() {
+if (this.maxTasksInQueue > 0 && this.queue.size() >=
this.maxTasksInQueue) {
+throw new RejectedExecutionException("Queue at limit of " +
this.maxTasksInQueue + " items");
+}
+}
+
+@Override
+public List> invokeAll(Collection>
tasks) throws InterruptedException {
+checkQueue();
Review comment:
Fixed
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 #80: BOOKKEEPER-816: use native fallocate to improve journal allocation
eolivelli commented on a change in pull request #80: BOOKKEEPER-816: use native
fallocate to improve journal allocation
URL: https://github.com/apache/bookkeeper/pull/80#discussion_r178185815
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java
##
@@ -797,6 +805,8 @@ public void scanJournal(long journalId, long journalPos,
JournalScanner scanner)
if (!isPaddingRecord) {
scanner.process(journalVersion, offset, recBuff);
}
+// update last log mark during replaying
+lastLogMark.setCurLogMark(journalId, offset);
}
Review comment:
This seems unrelated to this change
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 #80: BOOKKEEPER-816: use native fallocate to improve journal allocation
eolivelli commented on a change in pull request #80: BOOKKEEPER-816: use native
fallocate to improve journal allocation
URL: https://github.com/apache/bookkeeper/pull/80#discussion_r178185463
##
File path: bookkeeper-server/pom.xml
##
@@ -206,9 +206,96 @@
org.apache.maven.plugins
maven-checkstyle-plugin
+
+org.apache.maven.plugins
+maven-surefire-plugin
+${maven-surefire-plugin.version}
+
+ -Xmx2G -Djava.net.preferIPv4Stack=true
-Djava.library.path=${project.build.directory}/native/target/usr/local/lib
+ true
+ false
+ 1800
+ 2
+
+
+
+ native
+
+
+ native
+
+
+
+
+
+org.apache.maven.plugins
+maven-enforcer-plugin
+
+
+enforce-os
+
+ enforce
+
+
+
+
+ mac
+ unix
+ native build only supported on Mac or
Unix
+
+
+ true
+
+
+
+
+
+org.codehaus.mojo
+native-maven-plugin
+
+
+compile
+
+ javah
Review comment:
I will take a look into this. In jdk10 they dropped javah, in favour of
javac.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1309: Refactored OrderedSafeExecutor and OrderedScheduler
merlimat commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178186391
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java
##
@@ -0,0 +1,529 @@
+/**
+ * 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.common.util;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.stats.Gauge;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.stats.OpStatsLogger;
+import org.apache.bookkeeper.stats.StatsLogger;
+import org.apache.commons.lang.StringUtils;
+
+/**
+ * This class provides 2 things over the java {@link ExecutorService}.
+ *
+ * 1. It takes {@link SafeRunnable objects} instead of plain Runnable
objects.
+ * This means that exceptions in scheduled tasks wont go unnoticed and will be
+ * logged.
+ *
+ * 2. It supports submitting tasks with an ordering key, so that tasks
submitted
+ * with the same key will always be executed in order, but tasks across
+ * different keys can be unordered. This retains parallelism while retaining
the
+ * basic amount of ordering we want (e.g. , per ledger handle). Ordering is
+ * achieved by hashing the key objects to threads by their {@link #hashCode()}
+ * method.
+ */
+@Slf4j
+public class OrderedExecutor implements ExecutorService {
+public static final int NO_TASK_LIMIT = -1;
+protected static final long WARN_TIME_MICRO_SEC_DEFAULT =
TimeUnit.SECONDS.toMicros(1);
+
+final String name;
+final ExecutorService threads[];
+final long threadIds[];
+final Random rand = new Random();
+final OpStatsLogger taskExecutionStats;
+final OpStatsLogger taskPendingStats;
+final boolean traceTaskExecution;
+final long warnTimeMicroSec;
+final int maxTasksInQueue;
+
+
+public static Builder newBuilder() {
+return new Builder();
+}
+
+/**
+ * A builder class for an OrderedSafeExecutor.
+ */
+public static class Builder extends AbstractBuilder {
+
+@Override
+public OrderedExecutor build() {
+if (null == threadFactory) {
+threadFactory = new
DefaultThreadFactory("bookkeeper-ordered-safe-executor");
+}
+return new OrderedExecutor(name, numThreads, threadFactory,
statsLogger,
+ traceTaskExecution,
warnTimeMicroSec, maxTasksInQueue);
+}
+}
+
+/**
+ * Builder to build ordered scheduler.
+ */
+public static class SchedulerBuilder extends
AbstractBuilder {}
+
+/**
+ * Abstract builder class to build {@link OrderedScheduler}.
+ */
+public abstract static class AbstractBuilder {
+protected String name = getClass().getSimpleName();
+protected int numThreads = Runtime.getRuntime().availableProcessors();
+protected ThreadFactory threadFactory = null;
+protected StatsLogger statsLogger = NullStatsLogger.INSTANCE;
+protected boolean traceTaskExecution = false;
+protected long warnTimeMicroSec = WARN_TIME_MICRO_SEC_DEFAULT;
+protected int maxTasksInQueue = NO_TASK_LIMIT;
+
+public AbstractBuilder name(String name) {
+this.name = name;
+return this;
+}
+
+p
[GitHub] merlimat commented on a change in pull request #1309: Refactored OrderedSafeExecutor and OrderedScheduler
merlimat commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178186082
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java
##
@@ -0,0 +1,529 @@
+/**
+ * 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.common.util;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.stats.Gauge;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.stats.OpStatsLogger;
+import org.apache.bookkeeper.stats.StatsLogger;
+import org.apache.commons.lang.StringUtils;
+
+/**
+ * This class provides 2 things over the java {@link ExecutorService}.
+ *
+ * 1. It takes {@link SafeRunnable objects} instead of plain Runnable
objects.
+ * This means that exceptions in scheduled tasks wont go unnoticed and will be
+ * logged.
+ *
+ * 2. It supports submitting tasks with an ordering key, so that tasks
submitted
+ * with the same key will always be executed in order, but tasks across
+ * different keys can be unordered. This retains parallelism while retaining
the
+ * basic amount of ordering we want (e.g. , per ledger handle). Ordering is
+ * achieved by hashing the key objects to threads by their {@link #hashCode()}
+ * method.
+ */
+@Slf4j
+public class OrderedExecutor implements ExecutorService {
+public static final int NO_TASK_LIMIT = -1;
+protected static final long WARN_TIME_MICRO_SEC_DEFAULT =
TimeUnit.SECONDS.toMicros(1);
+
+final String name;
+final ExecutorService threads[];
+final long threadIds[];
+final Random rand = new Random();
+final OpStatsLogger taskExecutionStats;
+final OpStatsLogger taskPendingStats;
+final boolean traceTaskExecution;
+final long warnTimeMicroSec;
+final int maxTasksInQueue;
+
+
+public static Builder newBuilder() {
+return new Builder();
+}
+
+/**
+ * A builder class for an OrderedSafeExecutor.
+ */
+public static class Builder extends AbstractBuilder {
+
+@Override
+public OrderedExecutor build() {
+if (null == threadFactory) {
+threadFactory = new
DefaultThreadFactory("bookkeeper-ordered-safe-executor");
+}
+return new OrderedExecutor(name, numThreads, threadFactory,
statsLogger,
+ traceTaskExecution,
warnTimeMicroSec, maxTasksInQueue);
+}
+}
+
+/**
+ * Builder to build ordered scheduler.
+ */
+public static class SchedulerBuilder extends
AbstractBuilder {}
+
+/**
+ * Abstract builder class to build {@link OrderedScheduler}.
+ */
+public abstract static class AbstractBuilder {
Review comment:
Yes, it could be package private
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 #1309: Refactored OrderedSafeExecutor and OrderedScheduler
eolivelli commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178182068
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java
##
@@ -0,0 +1,529 @@
+/**
+ * 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.common.util;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.stats.Gauge;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.stats.OpStatsLogger;
+import org.apache.bookkeeper.stats.StatsLogger;
+import org.apache.commons.lang.StringUtils;
+
+/**
+ * This class provides 2 things over the java {@link ExecutorService}.
+ *
+ * 1. It takes {@link SafeRunnable objects} instead of plain Runnable
objects.
+ * This means that exceptions in scheduled tasks wont go unnoticed and will be
+ * logged.
+ *
+ * 2. It supports submitting tasks with an ordering key, so that tasks
submitted
+ * with the same key will always be executed in order, but tasks across
+ * different keys can be unordered. This retains parallelism while retaining
the
+ * basic amount of ordering we want (e.g. , per ledger handle). Ordering is
+ * achieved by hashing the key objects to threads by their {@link #hashCode()}
+ * method.
+ */
+@Slf4j
+public class OrderedExecutor implements ExecutorService {
+public static final int NO_TASK_LIMIT = -1;
+protected static final long WARN_TIME_MICRO_SEC_DEFAULT =
TimeUnit.SECONDS.toMicros(1);
+
+final String name;
+final ExecutorService threads[];
+final long threadIds[];
+final Random rand = new Random();
+final OpStatsLogger taskExecutionStats;
+final OpStatsLogger taskPendingStats;
+final boolean traceTaskExecution;
+final long warnTimeMicroSec;
+final int maxTasksInQueue;
+
+
+public static Builder newBuilder() {
+return new Builder();
+}
+
+/**
+ * A builder class for an OrderedSafeExecutor.
+ */
+public static class Builder extends AbstractBuilder {
+
+@Override
+public OrderedExecutor build() {
+if (null == threadFactory) {
+threadFactory = new
DefaultThreadFactory("bookkeeper-ordered-safe-executor");
+}
+return new OrderedExecutor(name, numThreads, threadFactory,
statsLogger,
+ traceTaskExecution,
warnTimeMicroSec, maxTasksInQueue);
+}
+}
+
+/**
+ * Builder to build ordered scheduler.
+ */
+public static class SchedulerBuilder extends
AbstractBuilder {}
+
+/**
+ * Abstract builder class to build {@link OrderedScheduler}.
+ */
+public abstract static class AbstractBuilder {
Review comment:
Why public?
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 #1309: Refactored OrderedSafeExecutor and OrderedScheduler
eolivelli commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178181634
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/BoundedExecutorService.java
##
@@ -0,0 +1,109 @@
+/**
+ * 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.common.util;
+
+import com.google.common.util.concurrent.ForwardingExecutorService;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * Implements {@link ExecutorService} and allows limiting the number of tasks
to
+ * be scheduled in the thread's queue.
+ */
+public class BoundedExecutorService extends ForwardingExecutorService {
+private final BlockingQueue queue;
+private final ThreadPoolExecutor thread;
+private final int maxTasksInQueue;
+
+public BoundedExecutorService(ThreadPoolExecutor thread, int
maxTasksInQueue) {
+this.queue = thread.getQueue();
+this.thread = thread;
+this.maxTasksInQueue = maxTasksInQueue;
+}
+
+@Override
+protected ExecutorService delegate() {
+return this.thread;
+}
+
+private void checkQueue() {
+if (this.maxTasksInQueue > 0 && this.queue.size() >=
this.maxTasksInQueue) {
+throw new RejectedExecutionException("Queue at limit of " +
this.maxTasksInQueue + " items");
+}
+}
+
+@Override
+public List> invokeAll(Collection>
tasks) throws InterruptedException {
+checkQueue();
Review comment:
Shouldn't you pass something like the size of the list? checkQueue assumes
you are going to add 1 item.
I mean something like
checkQueue(tasks.size())
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 #1309: Refactored OrderedSafeExecutor and OrderedScheduler
eolivelli commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178182518
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java
##
@@ -0,0 +1,529 @@
+/**
+ * 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.common.util;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.stats.Gauge;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.stats.OpStatsLogger;
+import org.apache.bookkeeper.stats.StatsLogger;
+import org.apache.commons.lang.StringUtils;
+
+/**
+ * This class provides 2 things over the java {@link ExecutorService}.
+ *
+ * 1. It takes {@link SafeRunnable objects} instead of plain Runnable
objects.
+ * This means that exceptions in scheduled tasks wont go unnoticed and will be
+ * logged.
+ *
+ * 2. It supports submitting tasks with an ordering key, so that tasks
submitted
+ * with the same key will always be executed in order, but tasks across
+ * different keys can be unordered. This retains parallelism while retaining
the
+ * basic amount of ordering we want (e.g. , per ledger handle). Ordering is
+ * achieved by hashing the key objects to threads by their {@link #hashCode()}
+ * method.
+ */
+@Slf4j
+public class OrderedExecutor implements ExecutorService {
+public static final int NO_TASK_LIMIT = -1;
+protected static final long WARN_TIME_MICRO_SEC_DEFAULT =
TimeUnit.SECONDS.toMicros(1);
+
+final String name;
+final ExecutorService threads[];
+final long threadIds[];
+final Random rand = new Random();
+final OpStatsLogger taskExecutionStats;
+final OpStatsLogger taskPendingStats;
+final boolean traceTaskExecution;
+final long warnTimeMicroSec;
+final int maxTasksInQueue;
+
+
+public static Builder newBuilder() {
+return new Builder();
+}
+
+/**
+ * A builder class for an OrderedSafeExecutor.
+ */
+public static class Builder extends AbstractBuilder {
+
+@Override
+public OrderedExecutor build() {
+if (null == threadFactory) {
+threadFactory = new
DefaultThreadFactory("bookkeeper-ordered-safe-executor");
+}
+return new OrderedExecutor(name, numThreads, threadFactory,
statsLogger,
+ traceTaskExecution,
warnTimeMicroSec, maxTasksInQueue);
+}
+}
+
+/**
+ * Builder to build ordered scheduler.
+ */
+public static class SchedulerBuilder extends
AbstractBuilder {}
+
+/**
+ * Abstract builder class to build {@link OrderedScheduler}.
+ */
+public abstract static class AbstractBuilder {
+protected String name = getClass().getSimpleName();
+protected int numThreads = Runtime.getRuntime().availableProcessors();
+protected ThreadFactory threadFactory = null;
+protected StatsLogger statsLogger = NullStatsLogger.INSTANCE;
+protected boolean traceTaskExecution = false;
+protected long warnTimeMicroSec = WARN_TIME_MICRO_SEC_DEFAULT;
+protected int maxTasksInQueue = NO_TASK_LIMIT;
+
+public AbstractBuilder name(String name) {
+this.name = name;
+return this;
+}
+
+
[GitHub] merlimat commented on issue #1292: Avoid acquiring closeLock.readLock() on every add/read operation
merlimat commented on issue #1292: Avoid acquiring closeLock.readLock() on every add/read operation URL: https://github.com/apache/bookkeeper/pull/1292#issuecomment-377370475 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] merlimat commented on issue #1292: Avoid acquiring closeLock.readLock() on every add/read operation
merlimat commented on issue #1292: Avoid acquiring closeLock.readLock() on every add/read operation URL: https://github.com/apache/bookkeeper/pull/1292#issuecomment-377370475 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] reddycharan commented on issue #1281: Issue #570: Introducing EntryLogManager.
reddycharan commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377367540 > If the movement makes it tricky for the multiple entrylog, that implies that the multiple entry log is adding ever more cross dependencies, which makes it even more important to sort it out early. EntryLogManager code/logic for entrylogperledger is going to be just one inner class in EntryLogManager - EntryLogManagerForEntryLogPerLedger. https://github.com/apache/bookkeeper/pull/1201/files#diff-66d58c001864b142c348f9191407c410R988 It doesn't add anymore cross dependencies. > As I said above, I'm fine with a refactor going in after this PR, but it has to go in before any more code goes in on top of it. I can work on the class refactoring as you highlighted (still i need to check if it is right to refactor the code as you have done) once this code and EntryLogManagerForEntryLogPerLedger (inner class with no additional cross dependencies) are in. > I can add "getCurrentLogId" to the EntryLogManager Interface. For EntryLogManagerForSingleEntryLog implementation it would give the logId of the current activelog (which is current behavior). @sijie if you are ok with my comment regarding currentLogId, I can send next iteration change. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on a change in pull request #1309: Refactored OrderedSafeExecutor and OrderedScheduler
merlimat commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178176919
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedSafeExecutor.java
##
@@ -0,0 +1,529 @@
+/**
+ * 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.common.util;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.stats.Gauge;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.stats.OpStatsLogger;
+import org.apache.bookkeeper.stats.StatsLogger;
+import org.apache.commons.lang.StringUtils;
+
+/**
+ * This class provides 2 things over the java {@link ExecutorService}.
+ *
+ * 1. It takes {@link SafeRunnable objects} instead of plain Runnable
objects.
+ * This means that exceptions in scheduled tasks wont go unnoticed and will be
+ * logged.
+ *
+ * 2. It supports submitting tasks with an ordering key, so that tasks
submitted
+ * with the same key will always be executed in order, but tasks across
+ * different keys can be unordered. This retains parallelism while retaining
the
+ * basic amount of ordering we want (e.g. , per ledger handle). Ordering is
+ * achieved by hashing the key objects to threads by their {@link #hashCode()}
+ * method.
+ */
+@Slf4j
+public class OrderedSafeExecutor implements ExecutorService {
Review comment:
Yes, makes sense.
Also, regarding `SafeRunnable`, I think we should be doing that internally,
just taking a `Runnable`, though that's another story from this PR.
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 #1308: longPollThreadPool can not be null
sijie commented on a change in pull request #1308: longPollThreadPool can not
be null
URL: https://github.com/apache/bookkeeper/pull/1308#discussion_r178171278
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
##
@@ -1288,10 +1288,15 @@ public ServerConfiguration
setNumLongPollWorkerThreads(int numThreads) {
/**
* Get the number of threads that should handle long poll requests.
- * @return
+ *
+ * If the number of threads is zero or negative, bookie will fallback to
+ * use read threads. If there is no read threads used, it will create a
thread pool
+ * with {@link Runtime#availableProcessors()} threads.
+ *
+ * @return the number of threads that should handle long poll requests,
default value is 0.
*/
public int getNumLongPollWorkerThreads() {
-return getInt(NUM_LONG_POLL_WORKER_THREADS, 10);
+return getInt(NUM_LONG_POLL_WORKER_THREADS, 0);
Review comment:
@yzang
good point.
this change attempts to allow not creating those threads or fallback to use
read thread pool for most of the use cases - either they don't use long poll
features (e.g. pulsar), or the requirement for separating long poll reads and
catch up reads is not that strong.
for a better I/O isolation, write/read/longpoll reads should usually be
isolated into different I/O thread pools. at twitter, I think all of our
configurations have set this value to 2~3 times of number of available
processor. so this change won't affect there.
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 #80: (WIP) BOOKKEEPER-816: use native fallocate to improve journal allocation
sijie commented on issue #80: (WIP) BOOKKEEPER-816: use native fallocate to improve journal allocation URL: https://github.com/apache/bookkeeper/pull/80#issuecomment-377358700 - Enable loading native library on tests … - Picked up @eolivelli fix eolivelli/bookkeeper@4c7e571 this PR should be ready for review now. 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] yzang commented on a change in pull request #1308: longPollThreadPool can not be null
yzang commented on a change in pull request #1308: longPollThreadPool can not
be null
URL: https://github.com/apache/bookkeeper/pull/1308#discussion_r178168776
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
##
@@ -1288,10 +1288,15 @@ public ServerConfiguration
setNumLongPollWorkerThreads(int numThreads) {
/**
* Get the number of threads that should handle long poll requests.
- * @return
+ *
+ * If the number of threads is zero or negative, bookie will fallback to
+ * use read threads. If there is no read threads used, it will create a
thread pool
+ * with {@link Runtime#availableProcessors()} threads.
+ *
+ * @return the number of threads that should handle long poll requests,
default value is 0.
*/
public int getNumLongPollWorkerThreads() {
-return getInt(NUM_LONG_POLL_WORKER_THREADS, 10);
+return getInt(NUM_LONG_POLL_WORKER_THREADS, 0);
Review comment:
What's the benefit of using 0 as the default value? Wouldn't it cause long
poll read interfere with catch up read?
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 #1309: Refactored OrderedSafeExecutor and OrderedScheduler
sijie commented on a change in pull request #1309: Refactored
OrderedSafeExecutor and OrderedScheduler
URL: https://github.com/apache/bookkeeper/pull/1309#discussion_r178165445
##
File path:
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedSafeExecutor.java
##
@@ -0,0 +1,529 @@
+/**
+ * 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.common.util;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+
+import io.netty.util.concurrent.DefaultThreadFactory;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.stats.Gauge;
+import org.apache.bookkeeper.stats.NullStatsLogger;
+import org.apache.bookkeeper.stats.OpStatsLogger;
+import org.apache.bookkeeper.stats.StatsLogger;
+import org.apache.commons.lang.StringUtils;
+
+/**
+ * This class provides 2 things over the java {@link ExecutorService}.
+ *
+ * 1. It takes {@link SafeRunnable objects} instead of plain Runnable
objects.
+ * This means that exceptions in scheduled tasks wont go unnoticed and will be
+ * logged.
+ *
+ * 2. It supports submitting tasks with an ordering key, so that tasks
submitted
+ * with the same key will always be executed in order, but tasks across
+ * different keys can be unordered. This retains parallelism while retaining
the
+ * basic amount of ordering we want (e.g. , per ledger handle). Ordering is
+ * achieved by hashing the key objects to threads by their {@link #hashCode()}
+ * method.
+ */
+@Slf4j
+public class OrderedSafeExecutor implements ExecutorService {
Review comment:
Since we are in a new package, can you rename it to "OrderedExecutor" to be
consistent with "OrderedScheduler"? This also make a distinguish from the old
`OrderedSafeExecutor`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat opened a new pull request #1309: Refactored OrderedSafeExecutor and OrderedScheduler
merlimat opened a new pull request #1309: Refactored OrderedSafeExecutor and OrderedScheduler URL: https://github.com/apache/bookkeeper/pull/1309 As outlined in https://lists.apache.org/thread.html/102383ea42f473f36720637e41af0ee83fc38d9f992736e0d1a7f985@%3Cdev.bookkeeper.apache.org%3E right now `OrderedSafeExecutor` is implemented on top of `OrderedScheduler`. There are few problems with this approach that are causing impact on performance: 1. `OrderedScheduler` is a `ScheduledExecutorService` which uses a priority queue for tasks. The priority queue has a single mutex for both publishers/consumers on the queue 2. There are many objects created for each task submission, due to listenable future decorators Since in all cases in critical write/read path we don't need delay task execution or futures, we should try to have a light weight execution for that. ### Modifications * Inverted the hierarchy between `OrderedSafeExecutor` and `OrderedScheduler`. Now the base class is `OrderedSafeExecutor` and the other extends from it, since it provides additional methods. * Moved `OrderedSafeExecutor` in `bookkeeper-common` since `OrderedScheduler` was already there. * Moved `OrderedSafeGenericCallback` in its own file, since it needs to be in `bookkeeper-server` module at this point. * Changed some method names from `submitOrdered()` into `executeOrdered()` to be consistent with JDK name (`submit()` returns a future while `execute()` returns void). * Changed `BookKeeper` instance of `scheduler` into `OrderedScheduler` so that the few cases which were using the `mainWorkerPool` could be easily converted to use the scheduler instead. 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 #1308: longPollThreadPool can not be null
sijie commented on issue #1308: longPollThreadPool can not be null URL: https://github.com/apache/bookkeeper/pull/1308#issuecomment-377346484 /cc @philipsu522 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 #1308: longPollThreadPool can not be null
sijie opened a new pull request #1308: longPollThreadPool can not be null URL: https://github.com/apache/bookkeeper/pull/1308 Descriptions of the changes in this PR: *Problem* The long polling logic is built with the assumption that there is a thread pool for scheduling deferred reads. So if people happens to set `numLongPollWorkerThreads` to zero or negative, a null value is passed into long poll requests which causes NPE. *Solution* If `numLongPollWorkerThreads` is set to zero or negative, fall back to use read thread pool. If there is no read thread pool, create a thread pool aligned with num processors. With this change, turn the default value of `long poll threads` to zero. so no additional threads are needed to be created if long poll feature is not used (e.g. at pulsar). > --- > 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 #1281: Issue #570: Introducing EntryLogManager.
sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377337977 @ivankelly > but if you are trying to use the argument on that JIRA as an example of where arguing for certain engineering standard lead to a bug > And what I was arguing against was exactly what caused the bug. what happened already happened. I wasn't and am not going to argue on that. What I was trying to say in my comment - focus were lost when there was too many arguments around the place. Again the purpose of my comment to bring the reviews focus back. > If I'm going to review code, I'm going to review it to the best of my ability to ensure that the code is maintainable in the future. If my reviews are unwanted, I can spend my time on other projects. we all want to the code to be maintainable. as what Charan pointed out, his sequence of PRs leads to maintainable - 1) abstract the code/logic for singleentrylog and entrylogperledger and move the implementations to EntryLogManager. 2) do the refactor to clean up the class hierarchicy. my very [first review comment](https://github.com/apache/bookkeeper/pull/1281#issuecomment-376311538) also suggested the similar thing, which will make the code maintainable. You review comments after that were kind of suggesting doing the reverse. It is not a problem of who is right or who is wrong, either sequence can lead to same result. What I am seeing wrong here is the different opinions on the sequence - which change goes in first and which change goes in after. That's the huge concern to me. The purpose of my previous comment to bring the focus back, so everyone is following same sequence (Charan's change sequence) on reviews, otherwise nothing is moving. 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] jiazhai closed pull request #1307: Use reflection based CRC32 to pass direct memory pointer
jiazhai closed pull request #1307: Use reflection based CRC32 to pass direct
memory pointer
URL: https://github.com/apache/bookkeeper/pull/1307
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java
index 1f066405a..26508280e 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/CRC32DigestManager.java
@@ -19,13 +19,30 @@
*/
import io.netty.buffer.ByteBuf;
-import java.util.zip.CRC32;
+import io.netty.util.concurrent.FastThreadLocal;
+/**
+ * Digest manager for CRC32 checksum.
+ */
class CRC32DigestManager extends DigestManager {
-private final ThreadLocal crc = new ThreadLocal() {
+
+/**
+ * Interface that abstracts different implementations of the CRC32 digest.
+ */
+interface CRC32Digest {
+long getValueAndReset();
+
+void update(ByteBuf buf);
+}
+
+private static final FastThreadLocal crc = new
FastThreadLocal() {
@Override
-protected CRC32 initialValue() {
-return new CRC32();
+protected CRC32Digest initialValue() {
+if (DirectMemoryCRC32Digest.isSupported()) {
+return new DirectMemoryCRC32Digest();
+} else {
+return new StandardCRC32Digest();
+}
}
};
@@ -40,12 +57,11 @@ int getMacCodeLength() {
@Override
void populateValueAndReset(ByteBuf buf) {
-buf.writeLong(crc.get().getValue());
-crc.get().reset();
+buf.writeLong(crc.get().getValueAndReset());
}
@Override
void update(ByteBuf data) {
-crc.get().update(data.nioBuffer());
+crc.get().update(data);
}
}
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java
new file mode 100644
index 0..a895153f5
--- /dev/null
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/checksum/DirectMemoryCRC32Digest.java
@@ -0,0 +1,94 @@
+/*
+* 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.proto.checksum;
+
+import io.netty.buffer.ByteBuf;
+
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.zip.CRC32;
+
+import org.apache.bookkeeper.proto.checksum.CRC32DigestManager.CRC32Digest;
+
+/**
+ * Specialized implementation of CRC32 digest that uses reflection on {@link
CRC32} class to get access to
+ * "updateByteBuffer" method and pass a direct memory pointer.
+ */
+class DirectMemoryCRC32Digest implements CRC32Digest {
+
+public static boolean isSupported() {
+return updateBytes != null;
+}
+
+private int crcValue;
+
+@Override
+public long getValueAndReset() {
+long value = crcValue & 0xL;
+crcValue = 0;
+return value;
+}
+
+@Override
+public void update(ByteBuf buf) {
+int index = buf.readerIndex();
+int length = buf.readableBytes();
+
+try {
+if (buf.hasMemoryAddress()) {
+// Calculate CRC directly from the direct memory pointer
+crcValue = (int) updateByteBuffer.invoke(null, crcValue,
buf.memoryAddress(), index, length);
+} else if (buf.hasArray()) {
+// Use the internal method to update from array based
+crcValue = (int) updateBytes.invoke(null, crcValue,
buf.array(), buf.arrayOffset() + index, length);
+} else {
+// Fallback to data copy if buffer is not contiguous
+byte[] b = new byte[length];
+buf.getBytes(index, b, 0, length);
+
[GitHub] ivankelly commented on issue #1281: Issue #570: Introducing EntryLogManager.
ivankelly commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377207411 @sijie > I hate to say this, but still I have to say it. because I don't want to get into these kind of arguments about "engineering-preferences" style arguments (e.g. circular dependency, two booleans) again and again. These are not '"engineering-preferences" style arguments'. boolean parameters and circular dependencies are well established as bad practices. > I still remembered a similar "circular dependency" argument that lasted for weeks when I introduced a better checkpoint mechanism 3-4 years ago. You disliked whatever you called "circular dependency" in the patch and let me change it to a way you preferred. but the approach turned out to have a correctness issue, and I had to revert it back (#659) recently to my original change that I did for Twitter (which has been running perfectly at twitter production for years). We could have been avoiding this kind of correctness issue if the review was more focused on logic and correctness rather than just thinking of breaking "circular dependency". Since you brought it up, my objection to BOOKKEEPER-564 was that journal was calling to ledger storage and then ledger storage was then calling to journal. This is _exactly_ where the issue in #659 came from. My initial comments on [BOOKKEPER-564](https://issues.apache.org/jira/browse/BOOKKEEPER-564) were that [the mark should travel with the entries](https://issues.apache.org/jira/browse/BOOKKEEPER-564?focusedCommentId=13623992&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13623992). Anyhow, not to go into too much detail, but if you are trying to use the argument on that JIRA as an example of where arguing for certain engineering standard lead to a bug, you're pretty wide of the mark. SortedLedgerStorage wasn't even in the branch at that point. And what I was arguing against was exactly what caused the bug. > Because I see these review comments (around moving classes, breaking circular dependencies) generally are not really helpful. If I'm going to review code, I'm going to review it to the best of my ability to ensure that the code is maintainable in the future. If my reviews are unwanted, I can spend my time on other projects. 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.
sijie commented on issue #1281: Issue #570: Introducing EntryLogManager. URL: https://github.com/apache/bookkeeper/pull/1281#issuecomment-377198776 @ivankelly > I'm of the opposite view. If the responsibility of each class is not clear, then you cannot possibly have a clean interface abstraction. I hate to say this, but still I have to say it. because I don't want to get into these kind of arguments about "engineering-preferences" style arguments (e.g. circular dependency, two booleans) again and again. I still remembered a similar "circular dependency" argument that lasted for weeks when I introduced a better checkpoint mechanism 3-4 years ago. You disliked whatever you called "circular dependency" in the patch and let me change it to a way you preferred. but the approach turned out to have a correctness issue, and I had to revert it back (#659) recently to my original change that I did for Twitter (which has been running perfectly at twitter production for years). We could have been avoiding this kind of correctness issue if the review was more focused on logic and correctness rather than just thinking of breaking "circular dependency". I am raising this old case here is not to point fingers. I want us to learn a lesson there, and avoid such case happening again. Because I see these review comments (around moving classes, breaking circular dependencies) generally are not really helpful. first, they make reviews losing focus; second, they have nothing to help with engineering progress; third, it exhausts energies for everyone involving in the PR - the author has the change his mind to one reviewer's mind, the other reviewers have to change their also, and etc. so I would be much happier to see if the reviews made are more focused on finding the correctness/logic issues with how the methods defined in this interface, rather than getting into yet-another-styleish-argument. 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.
ivankelly commented on a change in pull request #1281: Issue #570: Introducing
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r177986877
##
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:
If you have to supply a flag, then there's something external to be observed.
Let me rephrase. As I understand it, we do not want the behaviour of
SortedLedgerStorage flushing to change because of the change in this PR. So why
are we adding a new configuration 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] ivankelly commented on a change in pull request #1281: Issue #570: Introducing EntryLogManager.
ivankelly commented on a change in pull request #1281: Issue #570: Introducing
EntryLogManager.
URL: https://github.com/apache/bookkeeper/pull/1281#discussion_r177987104
##
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.
I agree about the methods providing the right abstraction. I was under the
impression that #getPreviousAllocatedEntryLogId() would always return an
incremented value if the current log has changed, but having read the code,
this is not the case.
It seems to me that SortedLedgerStorage is reaching very far into the
internals of the entrylogger to do what it wants to do. Since this is the case,
we shouldn't try to shoehorn this into the abstraction. SortedLedgerStorage
assumes that it uses a single log, so it should only ever be constructed with a
SingleLogEntryLogmanager, and SingleLogEntryLogManager should expose the stuff
that SortedLedgerStorage has required until now.
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
