You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/10/19 06:17:18 UTC
[GitHub] [bookkeeper] gaozhangmin opened a new pull request, #3554: Add watcher to watch isLedgerReplicationEnabled znode
gaozhangmin opened a new pull request, #3554:
URL: https://github.com/apache/bookkeeper/pull/3554
Fixes #3358
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1006405690
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java:
##########
@@ -1328,18 +1326,8 @@ void checkAllLedgers() throws BKException, IOException, InterruptedException {
final CompletableFuture<Void> processFuture = new CompletableFuture<>();
Processor<Long> checkLedgersProcessor = (ledgerId, callback) -> {
- try {
- if (!ledgerUnderreplicationManager.isLedgerReplicationEnabled()) {
- LOG.info("Ledger rereplication has been disabled, aborting periodic check");
- FutureUtils.complete(processFuture, null);
- return;
- }
- } catch (ReplicationException.NonRecoverableReplicationException nre) {
- LOG.error("Non Recoverable Exception while reading from ZK", nre);
- submitShutdownTask();
- return;
Review Comment:
it's handled where `notifyLedgerReplicationStatusChanged` is called @dlg99
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1005443995
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java:
##########
@@ -167,6 +167,7 @@ public class Auditor implements AutoCloseable {
private final int zkOpTimeoutMs;
private final Semaphore openLedgerNoRecoverySemaphore;
private final int openLedgerNoRecoverySemaphoreWaitTimeoutMSec;
+ private boolean isLedgerReplicationEnabled;
Review Comment:
This should be a AtomicBoolean as it is accessed by multiple threads
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1006041386
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java:
##########
@@ -737,6 +737,40 @@ public boolean isLedgerReplicationEnabled()
}
}
+ @Override
+ public void notifyLedgerReplicationStatusChanged(final GenericCallback<Void> cb)
+ throws ReplicationException.UnavailableException {
Review Comment:
ReplicationException.fromKeeperException can throw NonRecoverableReplicationException
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java:
##########
@@ -818,11 +821,6 @@ public void run() {
LOG.error("Exception running periodic check", bke);
} catch (IOException ioe) {
LOG.error("I/O exception running periodic check", ioe);
- } catch (ReplicationException.NonRecoverableReplicationException nre) {
- LOG.error("Non Recoverable Exception while reading from ZK", nre);
- submitShutdownTask();
Review Comment:
why is this removed / where NonRecoverableReplicationException is handled now?
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java:
##########
@@ -1328,18 +1326,8 @@ void checkAllLedgers() throws BKException, IOException, InterruptedException {
final CompletableFuture<Void> processFuture = new CompletableFuture<>();
Processor<Long> checkLedgersProcessor = (ledgerId, callback) -> {
- try {
- if (!ledgerUnderreplicationManager.isLedgerReplicationEnabled()) {
- LOG.info("Ledger rereplication has been disabled, aborting periodic check");
- FutureUtils.complete(processFuture, null);
- return;
- }
- } catch (ReplicationException.NonRecoverableReplicationException nre) {
- LOG.error("Non Recoverable Exception while reading from ZK", nre);
- submitShutdownTask();
- return;
Review Comment:
why is this removed / where NonRecoverableReplicationException is handled now?
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerUnderreplicationManager.java:
##########
@@ -148,6 +148,16 @@ void enableLedgerReplication()
boolean isLedgerReplicationEnabled()
throws ReplicationException.UnavailableException;
+ /**
+ * Receive notification asynchronously when the ledger replication process
+ * is changed.
+ *
+ * @param cb
+ * - callback implementation to receive the notification
+ */
+ void notifyLedgerReplicationStatusChanged(GenericCallback<Void> cb)
Review Comment:
something like
``` java
default void notifyLedgerReplicationStatusChanged(GenericCallback<Void> cb) throws ReplicationException.UnavailableException {
throw new RuntimeException("not implemented");
}
```
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java:
##########
@@ -167,6 +167,7 @@ public class Auditor implements AutoCloseable {
private final int zkOpTimeoutMs;
private final Semaphore openLedgerNoRecoverySemaphore;
private final int openLedgerNoRecoverySemaphoreWaitTimeoutMSec;
+ private boolean isLedgerReplicationEnabled;
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1006405690
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java:
##########
@@ -1328,18 +1326,8 @@ void checkAllLedgers() throws BKException, IOException, InterruptedException {
final CompletableFuture<Void> processFuture = new CompletableFuture<>();
Processor<Long> checkLedgersProcessor = (ledgerId, callback) -> {
- try {
- if (!ledgerUnderreplicationManager.isLedgerReplicationEnabled()) {
- LOG.info("Ledger rereplication has been disabled, aborting periodic check");
- FutureUtils.complete(processFuture, null);
- return;
- }
- } catch (ReplicationException.NonRecoverableReplicationException nre) {
- LOG.error("Non Recoverable Exception while reading from ZK", nre);
- submitShutdownTask();
- return;
Review Comment:
it's handled where `notifyLedgerReplicationStatusChanged` is called
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #3554: Add watcher to watch isLedgerReplicationEnabled znode
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#issuecomment-1289971715
@dlg99 PTAL Thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1013092363
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerUnderreplicationManager.java:
##########
@@ -148,6 +148,18 @@ void enableLedgerReplication()
boolean isLedgerReplicationEnabled()
throws ReplicationException.UnavailableException;
+ /**
+ * Receive notification asynchronously when the ledger replication process
+ * is changed.
+ *
+ * @param cb
+ * - callback implementation to receive the notification
+ */
+ default void notifyLedgerReplicationStatusChanged(GenericCallback<Void> cb)
+ throws ReplicationException.UnavailableException {
+ throw new RuntimeException("not implemented");
Review Comment:
This would be breaking the (runtime) compatibility for an existing `LedgerUnderReplicationManager` that does not implement the method, since it will throw exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1005442515
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java:
##########
@@ -737,6 +737,40 @@ public boolean isLedgerReplicationEnabled()
}
}
+ @Override
+ public void notifyLedgerReplicationStatusChanged(final GenericCallback<Void> cb)
+ throws ReplicationException.UnavailableException {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("notifyLedgerReplicationStatusChanged()");
+ }
+ Watcher w = new Watcher() {
+ @Override
+ public void process(WatchedEvent e) {
+ if (e.getType() == Watcher.Event.EventType.NodeDeleted) {
+ LOG.info("aLedgerReplication is enabled externally through Zookeeper, "
+ + "since DISABLE_NODE ZNode is deleted");
+ cb.operationComplete(0, null);
+ } else if (e.getType() == Watcher.Event.EventType.NodeCreated) {
+ LOG.info("bLedgerReplication is disabled externally through Zookeeper, "
+ + "since DISABLE_NODE ZNode is created");
+ cb.operationComplete(1, null);
+ }
+ }
+ };
+ try {
+ zkc.addWatch(basePath + '/'
Review Comment:
We must implement this for every supported Metadata driver
What happens if the driver doesn't implement it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#issuecomment-1294527175
@eolivelli @dlg99 PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1006048918
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java:
##########
@@ -167,6 +167,7 @@ public class Auditor implements AutoCloseable {
private final int zkOpTimeoutMs;
private final Semaphore openLedgerNoRecoverySemaphore;
private final int openLedgerNoRecoverySemaphoreWaitTimeoutMSec;
+ private boolean isLedgerReplicationEnabled;
Review Comment:
+1 to making it Atomic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1013644641
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerUnderreplicationManager.java:
##########
@@ -148,6 +148,18 @@ void enableLedgerReplication()
boolean isLedgerReplicationEnabled()
throws ReplicationException.UnavailableException;
+ /**
+ * Receive notification asynchronously when the ledger replication process
+ * is changed.
+ *
+ * @param cb
+ * - callback implementation to receive the notification
+ */
+ default void notifyLedgerReplicationStatusChanged(GenericCallback<Void> cb)
+ throws ReplicationException.UnavailableException {
+ throw new RuntimeException("not implemented");
Review Comment:
Remove default method as current now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#issuecomment-1303057831
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on a diff in pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#discussion_r1005489479
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/ZkLedgerUnderreplicationManager.java:
##########
@@ -737,6 +737,40 @@ public boolean isLedgerReplicationEnabled()
}
}
+ @Override
+ public void notifyLedgerReplicationStatusChanged(final GenericCallback<Void> cb)
+ throws ReplicationException.UnavailableException {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("notifyLedgerReplicationStatusChanged()");
+ }
+ Watcher w = new Watcher() {
+ @Override
+ public void process(WatchedEvent e) {
+ if (e.getType() == Watcher.Event.EventType.NodeDeleted) {
+ LOG.info("aLedgerReplication is enabled externally through Zookeeper, "
+ + "since DISABLE_NODE ZNode is deleted");
+ cb.operationComplete(0, null);
+ } else if (e.getType() == Watcher.Event.EventType.NodeCreated) {
+ LOG.info("bLedgerReplication is disabled externally through Zookeeper, "
+ + "since DISABLE_NODE ZNode is created");
+ cb.operationComplete(1, null);
+ }
+ }
+ };
+ try {
+ zkc.addWatch(basePath + '/'
Review Comment:
If this, We also have this problem currently. see `notifyLedgerReplicationEnabled `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #3554:
URL: https://github.com/apache/bookkeeper/pull/3554#issuecomment-1293109972
rerun failure checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin closed pull request #3554: In avoid to call zk frequently, using watcher to watch the change
Posted by "gaozhangmin (via GitHub)" <gi...@apache.org>.
gaozhangmin closed pull request #3554: In avoid to call zk frequently, using watcher to watch the change
URL: https://github.com/apache/bookkeeper/pull/3554
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org