[GitHub] sijie commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r174219548 ## 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: @eolivelli it was already 1 - https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java#L338 I don't think the change touches it. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r174215629
##
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:
> it is returning void/null.
then you might consider returning Future
> MockExecutorController doesn't support controlSubmitCallable.
oh, we can add support for submit callables
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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r173617062 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java ## @@ -94,7 +94,7 @@ LedgerUnderreplicationManager newLedgerUnderreplicationManager() * @param lm *Layout manager */ Review comment: it is probably just need rebase. not a big deal, not sure it is worth a comment here. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r173616797 ## 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: why "\_"? we don't use "_" in the setting name. 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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r173616989
##
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:
I am fine with current implementation with boolean flag. but not against to
add a no-op start().
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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r173616847 ## 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, Review comment: no, it should be the issue link. because the issue link will contain all the review comments, but git sha doesn't contain any review comments. 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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r173616797 ## 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: why "_"? we don't use "_" in the setting name. 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 #1236: Issue #570: make changes to SyncThread/checkpoint logic.
sijie 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_r173259411
##
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:
Can you rename this to "scheduleCheckpointAtFixedRate(long flushIntervalMs)"
as I suggested in #1201?
- it is not actually a "start" action because the scheduler has been
started, this is more a "schedule" action
- flushInterval doesn't belong to SyncThread any more.
Also can you update the comment about 'flushInterval' in ServerConfiguration
and conf/bk_server.conf, `site/_data/config/bk_server.yaml`? `flushInterval` is
now only works when `entryLogPerLedger` enabled.
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
