[GitHub] reddycharan commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r174231934
##
File path:
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/SortedLedgerStorageCheckpointTest.java
##
@@ -110,15 +110,27 @@ public void setUp() throws Exception {
// initial checkpoint
this.storage = new SortedLedgerStorage();
-this.checkpointer = checkpoint -> storage.getScheduler().submit(() -> {
-log.info("Checkpoint the storage at {}", checkpoint);
-try {
-storage.checkpoint(checkpoint);
-checkpoints.add(checkpoint);
-} catch (IOException e) {
-log.error("Failed to checkpoint at {}", checkpoint, e);
+this.checkpointer = new Checkpointer() {
+@Override
+public void startCheckpoint(Checkpoint checkpoint) {
+// TODO Auto-generated method stub
Review comment:
removed 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] reddycharan commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic. URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r174220910 ## File path: site/_data/config/bk_server.yaml ## @@ -43,8 +43,8 @@ groups: description: Interval to watch whether bookie is dead or not, in milliseconds. default: 1000 - param: flushInterval -description: How long the interval to flush ledger index pages to disk, in milliseconds. Flushing index files will introduce much random disk I/O. If separating journal dir and ledger dirs each on different devices, flushing would not affect performance. But if putting journal dir and ledger dirs on same device, performance degrade significantly on too frequent flushing. You can consider increment flush interval to get better performance, but you need to pay more time on bookie server restart after failure. -default: 100 +description: When entryLogPerLedgerEnabled is enabled, checkpoint doesn't happens when a new active entrylog is created / previous one is rolled over. Instead SyncThread checkpoints periodically with 'flushInterval' delay (in milliseconds) in between executions. Checkpoint flushes both ledger entryLogs and ledger index pages to disk. Flushing entrylog and index files will introduce much random disk I/O. If separating journal dir and ledger dirs each on different devices, flushing would not affect performance. But if putting journal dir and ledger dirs on same device, performance degrade significantly on too frequent flushing. You can consider increment flush interval to get better performance, but you need to pay more time on bookie server restart after failure. This config is used only when entryLogPerLedgerEnabled is enabled. +default: 1 - param: allowStorageExpansion Review comment: there is discrepancy in the default value of this config. In ServerConfiguration.java it is mentioned as 1 (10 secs) but in this file it is mentioned as 100. So I'm correcting 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] reddycharan commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r174214945
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SyncThread.java
##
@@ -108,14 +103,13 @@ public void startCheckpoint(Checkpoint checkpoint) {
});
}
-public Future requestFlush() {
+public Future requestFlush() {
Review comment:
1) it is returning void/null.
2) its return value is not used anywhere.
3) MockExecutorController doesn't support controlSubmitCallable.
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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r174040801
##
File path:
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerStorageCheckpointTest.java
##
@@ -0,0 +1,580 @@
+/**
+ *
+ * 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.bookie;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.bookkeeper.bookie.Journal.LastLogMark;
+import org.apache.bookkeeper.client.BookKeeper;
+import org.apache.bookkeeper.client.BookKeeper.DigestType;
+import org.apache.bookkeeper.client.LedgerEntry;
+import org.apache.bookkeeper.client.LedgerHandle;
+import org.apache.bookkeeper.conf.ClientConfiguration;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.conf.TestBKConfiguration;
+import org.apache.bookkeeper.proto.BookieServer;
+import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
+import org.apache.bookkeeper.test.PortManager;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * LedgerStorageCheckpointTest.
+ *
+ */
+public class LedgerStorageCheckpointTest extends BookKeeperClusterTestCase {
+private static final Logger LOG = LoggerFactory
+.getLogger(LedgerStorageCheckpointTest.class);
+
+@Rule
+public final TestName runtime = new TestName();
+
+public LedgerStorageCheckpointTest() {
+super(0);
+}
+
+private LogMark readLastMarkFile(File lastMarkFile) throws IOException {
+byte buff[] = new byte[16];
+ByteBuffer bb = ByteBuffer.wrap(buff);
+LogMark rolledLogMark = new LogMark();
+FileInputStream fis = new FileInputStream(lastMarkFile);
+int bytesRead = fis.read(buff);
+fis.close();
+if (bytesRead != 16) {
+throw new IOException("Couldn't read enough bytes from lastMark."
+ " Wanted " + 16 + ", got " + bytesRead);
+}
+bb.clear();
+rolledLogMark.readLogMark(bb);
+return rolledLogMark;
+}
+
+/*
+ * In this testcase, InterleavedLedgerStorage is used and validate if the
+ * checkpoint is called for every flushinterval period.
+ */
+@Test
+public void testPeriodicCheckpointForInterleavedLedgerStorage() throws
Exception {
+
testPeriodicCheckpointForLedgerStorage(InterleavedLedgerStorage.class.getName());
+}
+
+/*
+ * In this testcase, SortedLedgerStorage is used and validate if the
+ * checkpoint is called for every flushinterval period.
+ */
+@Test
+public void testPeriodicCheckpointForSortedLedgerStorage() throws
Exception {
+
testPeriodicCheckpointForLedgerStorage(SortedLedgerStorage.class.getName());
+}
+
+public void testPeriodicCheckpointForLedgerStorage(String
ledgerStorageClassName) throws Exception {
Review comment:
since it is Checkpoint logic, I want to be cautious. Hence added testing at
Bookie level. For each testcase I added comment describing what I'm intending
to do in the testcase.
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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r174040089
##
File path:
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/LedgerStorageCheckpointTest.java
##
@@ -0,0 +1,580 @@
+/**
+ *
+ * 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.bookie;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import org.apache.bookkeeper.bookie.Journal.LastLogMark;
+import org.apache.bookkeeper.client.BookKeeper;
+import org.apache.bookkeeper.client.BookKeeper.DigestType;
+import org.apache.bookkeeper.client.LedgerEntry;
+import org.apache.bookkeeper.client.LedgerHandle;
+import org.apache.bookkeeper.conf.ClientConfiguration;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.conf.TestBKConfiguration;
+import org.apache.bookkeeper.proto.BookieServer;
+import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
+import org.apache.bookkeeper.test.PortManager;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * LedgerStorageCheckpointTest.
+ *
+ */
+public class LedgerStorageCheckpointTest extends BookKeeperClusterTestCase {
+private static final Logger LOG = LoggerFactory
+.getLogger(LedgerStorageCheckpointTest.class);
+
+@Rule
+public final TestName runtime = new TestName();
+
+public LedgerStorageCheckpointTest() {
+super(0);
+}
+
+private LogMark readLastMarkFile(File lastMarkFile) throws IOException {
+byte buff[] = new byte[16];
+ByteBuffer bb = ByteBuffer.wrap(buff);
+LogMark rolledLogMark = new LogMark();
+FileInputStream fis = new FileInputStream(lastMarkFile);
+int bytesRead = fis.read(buff);
+fis.close();
+if (bytesRead != 16) {
+throw new IOException("Couldn't read enough bytes from lastMark."
+ " Wanted " + 16 + ", got " + bytesRead);
+}
+bb.clear();
+rolledLogMark.readLogMark(bb);
+return rolledLogMark;
+}
+
+/*
+ * In this testcase, InterleavedLedgerStorage is used and validate if the
+ * checkpoint is called for every flushinterval period.
+ */
+@Test
+public void testPeriodicCheckpointForInterleavedLedgerStorage() throws
Exception {
+
testPeriodicCheckpointForLedgerStorage(InterleavedLedgerStorage.class.getName());
+}
+
+/*
+ * In this testcase, SortedLedgerStorage is used and validate if the
+ * checkpoint is called for every flushinterval period.
+ */
+@Test
+public void testPeriodicCheckpointForSortedLedgerStorage() throws
Exception {
+
testPeriodicCheckpointForLedgerStorage(SortedLedgerStorage.class.getName());
+}
+
+public void testPeriodicCheckpointForLedgerStorage(String
ledgerStorageClassName) throws Exception {
+File tmpDir = createTempDir("DiskCheck", "test");
+
+final ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration()
+.setZkServers(zkUtil.getZooKeeperConnectString())
+.setZkTimeout(5000)
+.setJournalDirName(tmpDir.getPath())
+.setLedgerDirNames(new String[] { tmpDir.getPath() })
+.setAutoRecoveryDaemonEnabled(false)
+.setFlushInterval(2000)
+.setBookiePort(PortManager.nextFreePort())
+// entrylog per ledger is enabled
+.setEntryLogPerLedgerEnabled(true)
+.setLedgerStorageClass(ledgerStorageClassName);
+Assert.assertEquals("Number of JournalDirs", 1,
conf.getJournalDirs().length);
+// we
[GitHub] reddycharan commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r174039621
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
##
@@ -331,9 +333,16 @@ private void flushOrCheckpoint(boolean isCheckpointFlush)
}
try {
-// if it is just a checkpoint flush, we just flush rotated entry
log files
-// in entry logger.
-if (isCheckpointFlush) {
+/*
+ * if it is just a checkpoint flush and if entryLogPerLedger is not
+ * enabled, then we just flush rotated entry log files in entry
+ * logger.
+ *
+ * In the case of entryLogPerLedgerEnabled we need to flush both
+ * rotatedlogs and currentlogs. Hence we call entryLogger.flush in
+ * the case of entrylogperledgerenabled.
+ */
+if (isCheckpointFlush && !entryLogPerLedgerEnabled) {
Review comment:
for now moved this if-else block to EntryLogger.checkpoint. In the
subsequent tasks when I introduce EntrylogManager, I'll move the implementation
of checkpoint to individual implementations of EntryLogManager.
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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r174039005
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
##
@@ -696,14 +698,29 @@ public Bookie(ServerConfiguration conf, StatsLogger
statsLogger)
ledgerStorage =
LedgerStorageFactory.createLedgerStorage(ledgerStorageClass);
syncThread = new SyncThread(conf, getLedgerDirsListener(),
ledgerStorage, checkpointSource);
+Checkpointer checkpointer;
+/*
+ * with this change https://github.com/apache/bookkeeper/pull/677,
+ * LedgerStorage drives the checkpoint logic. But with multiple entry
+ * logs, checkpoint logic based on a entry log is not possible, hence
it
+ * needs to be timebased recurring thing and it is driven by
SyncThread.
+ * SyncThread.start does that and it is started in Bookie.start method.
+ */
+if (entryLogPerLedgerEnabled) {
Review comment:
added start method to Checkpointer interface.
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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic. URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r173972600 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java ## @@ -179,6 +179,11 @@ // Stats protected static final String ENABLE_TASK_EXECUTION_STATS = "enableTaskExecutionStats"; +/* + * config specifying if the entrylog per ledger is enabled or not. + */ +protected static final String ENTRY_LOG_PERLEDGER_ENABLED = "entryLogPerLedgerEnabled"; Review comment: changed 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] reddycharan commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r173575748
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
##
@@ -696,14 +698,29 @@ public Bookie(ServerConfiguration conf, StatsLogger
statsLogger)
ledgerStorage =
LedgerStorageFactory.createLedgerStorage(ledgerStorageClass);
syncThread = new SyncThread(conf, getLedgerDirsListener(),
ledgerStorage, checkpointSource);
+Checkpointer checkpointer;
+/*
+ * with this change https://github.com/apache/bookkeeper/pull/677,
+ * LedgerStorage drives the checkpoint logic. But with multiple entry
+ * logs, checkpoint logic based on a entry log is not possible, hence
it
+ * needs to be timebased recurring thing and it is driven by
SyncThread.
+ * SyncThread.start does that and it is started in Bookie.start method.
+ */
+if (entryLogPerLedgerEnabled) {
Review comment:
hmm, I've no strong opinion either way.. @sijie what do you think about
having no-op start() method in SyncThread class and Bookie calling .start
method in all cases?
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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
reddycharan commented on a change in pull request #1236: Issue #570: make
changes to SyncThread/checkpoint logic.
URL: https://github.com/apache/bookkeeper/pull/1236#discussion_r173297183
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SyncThread.java
##
@@ -119,6 +119,12 @@ public void startCheckpoint(Checkpoint checkpoint) {
});
}
+void start() {
Review comment:
will do.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
