You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/05/26 07:32:06 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request #10711: [Transaction] Fix transaction log handle managed ledger WriteFail state.
congbobo184 opened a new pull request #10711:
URL: https://github.com/apache/pulsar/pull/10711
## Motivation
when transaction log managed ledger state become WriteFailed state, should `readyToCreateNewLedger`.
## implement
append fail check the managedLedger state and the exception do `readyToCreateNewLedger`
### Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (yes)
Anything that affects deployment: (no)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #10711: [Transaction] Fix transaction log handle managed ledger WriteFail state.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #10711:
URL: https://github.com/apache/pulsar/pull/10711#discussion_r639496319
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -215,7 +215,7 @@
protected static final int DEFAULT_LEDGER_DELETE_RETRIES = 3;
protected static final int DEFAULT_LEDGER_DELETE_BACKOFF_TIME_SEC = 60;
- enum State {
+ public enum State {
Review comment:
It's better do not to expose the internal state of the managed ledger, you can add a method to check if the managed ledger is closed?
##########
File path: pulsar-transaction/coordinator/src/test/java/org/apache/pulsar/transaction/coordinator/MLTransactionMetadataStoreTest.java
##########
@@ -367,6 +370,38 @@ public void testRecoverWhenDeleteFromCursor() throws Exception {
Awaitility.await().until(transactionMetadataStore::checkIfReady);
}
+ @Test
+ public void testManageLedgerWriteFailState() throws Exception {
+ ManagedLedgerFactoryConfig factoryConf = new ManagedLedgerFactoryConfig();
+ factoryConf.setMaxCacheSize(0);
+
+ @Cleanup("shutdown")
+ ManagedLedgerFactory factory = new ManagedLedgerFactoryImpl(metadataStore, bkc, factoryConf);
+ TransactionCoordinatorID transactionCoordinatorID = new TransactionCoordinatorID(1);
+ MLTransactionLogImpl mlTransactionLog = new MLTransactionLogImpl(transactionCoordinatorID, factory,
+ new ManagedLedgerConfig());
+ MLTransactionMetadataStore transactionMetadataStore =
+ new MLTransactionMetadataStore(transactionCoordinatorID, mlTransactionLog,
+ new TransactionTimeoutTrackerImpl(), new TransactionRecoverTrackerImpl());
+
+ Awaitility.await().until(transactionMetadataStore::checkIfReady);
+ transactionMetadataStore.newTransaction(5000).get();
+ Field field = MLTransactionLogImpl.class.getDeclaredField("managedLedger");
+ field.setAccessible(true);
+ ManagedLedgerImpl managedLedger = (ManagedLedgerImpl) field.get(mlTransactionLog);
+ field = ManagedLedgerImpl.class.getDeclaredField("STATE_UPDATER");
+ field.setAccessible(true);
+ AtomicReferenceFieldUpdater state = (AtomicReferenceFieldUpdater) field.get(managedLedger);
+ state.set(managedLedger, WriteFailed);
+ try {
+ transactionMetadataStore.newTransaction(5000).get();
Review comment:
Should add fail() here?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #10711: [Transaction] Fix transaction log handle managed ledger WriteFail state.
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #10711:
URL: https://github.com/apache/pulsar/pull/10711#issuecomment-849205014
@congbobo184 does this need to add docs?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #10711: [Transaction] Fix transaction log handle managed ledger WriteFail state.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #10711:
URL: https://github.com/apache/pulsar/pull/10711#issuecomment-849237752
@Anonymitaet This is not needed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #10711: [Transaction] Fix transaction log handle managed ledger WriteFail state.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10711:
URL: https://github.com/apache/pulsar/pull/10711
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #10711: [Transaction] Fix transaction log handle managed ledger WriteFail state.
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #10711:
URL: https://github.com/apache/pulsar/pull/10711#discussion_r640225117
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
##########
@@ -324,10 +324,10 @@ public void asyncOpen(final String name, final ManagedLedgerConfig config, final
if (existingFuture.isDone()) {
try {
ManagedLedgerImpl l = existingFuture.get();
- if (l.getState().equals(State.Fenced.toString()) || l.getState().equals(State.Closed.toString())) {
+ if (l.getState() == State.Fenced || l.getState() == State.Closed) {
// Managed ledger is in unusable state. Recreate it.
log.warn("[{}] Attempted to open ledger in {} state. Removing from the map to recreate it",
- name, l.getState());
+ name, l.getState().toString());
Review comment:
I don't think this change is needed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org