You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Cyrill (via GitHub)" <gi...@apache.org> on 2024/01/31 09:54:25 UTC
[PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Cyrill opened a new pull request, #3128:
URL: https://github.com/apache/ignite-3/pull/3128
https://issues.apache.org/jira/browse/IGNITE-21371
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1474536054
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -170,6 +170,8 @@ public class InternalTableImpl implements InternalTable {
/** Placement driver. */
private final PlacementDriver placementDriver;
+ private final ClusterNode localNode;
Review Comment:
Please check my comment bellow. I believe that we should retrieve coordinatorId from txManager on tx begin, thus we will not add given field 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1476037265
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/TxFinishReplicaRequest.java:
##########
@@ -59,6 +59,13 @@ public interface TxFinishReplicaRequest extends PrimaryReplicaRequest, Timestamp
*/
long commitTimestampLong();
+ /**
+ * Get the transaction coordinator inconsistent ID.
+ *
+ * @return the transaction coordinator inconsistent ID
+ */
+ String txCoordinatorId();
Review Comment:
I mean logically. Why do we need coordinatorId on finish request? From my point of view, we use coordinatorId retrieved from txnState local map in two cases only.
1. In order to check whether coordinator is dead and trigger the recovey, which definitely has no sense in case of finished transaction.
2. In order to decrement counter of an "in-flight" transactions that is used within index build procedure. In that case it's guaranteed that txnState local map will be populated with txCoordId prior to finish processing.
All in all, I believe that we may remove txCoordinatorId() from TxFinishReplicaRequest and update
```
private void markFinished(UUID txId, boolean commit, @Nullable HybridTimestamp commitTimestamp) {
txManager.updateTxMeta(txId, old -> new TxStateMeta(
commit ? COMMITTED : ABORTED,
old == null ? null : old.txCoordinatorId(),
...
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1474629231
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxMessageSender.java:
##########
@@ -48,6 +49,9 @@ public class TxMessageSender {
/** Messaging service. */
private final MessagingService messagingService;
+ /** Topology service. */
+ private final TopologyService topologyService;
Review Comment:
I'd rather make move it to finish params list in a form of String txCoordId.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1476037265
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/TxFinishReplicaRequest.java:
##########
@@ -59,6 +59,13 @@ public interface TxFinishReplicaRequest extends PrimaryReplicaRequest, Timestamp
*/
long commitTimestampLong();
+ /**
+ * Get the transaction coordinator inconsistent ID.
+ *
+ * @return the transaction coordinator inconsistent ID
+ */
+ String txCoordinatorId();
Review Comment:
I mean logically. Why do we need coordinatorId on finish request? From my point of view, we use coordinatorId retrieved from txnState local map in two cases.
1. In order to check whether coordinator is dead and trigger the recovey, which definitely has no sense in case of finished transaction.
2. In order to decrement counter of an "in-flight" transactions that is used within index build procedure. In that case it's guaranteed that txnState local map will be populated with txCoordId prior to finish processing.
All in all, I believe that we may remove txCoordinatorId() from TxFinishReplicaRequest and update
```
private void markFinished(UUID txId, boolean commit, @Nullable HybridTimestamp commitTimestamp) {
txManager.updateTxMeta(txId, old -> new TxStateMeta(
commit ? COMMITTED : ABORTED,
old == null ? null : old.txCoordinatorId(),
...
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc merged PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1475771688
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -170,6 +170,8 @@ public class InternalTableImpl implements InternalTable {
/** Placement driver. */
private final PlacementDriver placementDriver;
+ private final ClusterNode localNode;
Review Comment:
done
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/TxMessageSender.java:
##########
@@ -48,6 +49,9 @@ public class TxMessageSender {
/** Messaging service. */
private final MessagingService messagingService;
+ /** Topology service. */
+ private final TopologyService topologyService;
Review Comment:
done
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1474530688
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -248,6 +254,10 @@ public void name(String newName) {
this.tableName = newName;
}
+ private String txCoordinatorId() {
Review Comment:
I'd rather add coordinatorId to InternalTransaction and thus remove given method from InternalTableImpl. What I mean here is that txManager should propagate coordinatorId to InternalTransaction on tx.begin().
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1474641670
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/TxFinishReplicaRequest.java:
##########
@@ -59,6 +59,13 @@ public interface TxFinishReplicaRequest extends PrimaryReplicaRequest, Timestamp
*/
long commitTimestampLong();
+ /**
+ * Get the transaction coordinator inconsistent ID.
+ *
+ * @return the transaction coordinator inconsistent ID
+ */
+ String txCoordinatorId();
Review Comment:
Do we really need txCoordinatorId in TxFinishReplicaRequest? Use-cases?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1476123672
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/TxFinishReplicaRequest.java:
##########
@@ -59,6 +59,13 @@ public interface TxFinishReplicaRequest extends PrimaryReplicaRequest, Timestamp
*/
long commitTimestampLong();
+ /**
+ * Get the transaction coordinator inconsistent ID.
+ *
+ * @return the transaction coordinator inconsistent ID
+ */
+ String txCoordinatorId();
Review Comment:
Same applies to WriteIntentSwitch command handling
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -69,8 +69,8 @@ public class ReadWriteTransactionImpl extends IgniteAbstractTransactionImpl {
* @param observableTsTracker Observable timestamp tracker.
* @param id The id.
*/
- public ReadWriteTransactionImpl(TxManager txManager, HybridTimestampTracker observableTsTracker, UUID id) {
- super(txManager, id);
+ public ReadWriteTransactionImpl(TxManager txManager, HybridTimestampTracker observableTsTracker, UUID id, String txCoordinatorId) {
Review Comment:
done
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1475771367
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replication/request/CommittableTxRequest.java:
##########
@@ -32,6 +32,13 @@ public interface CommittableTxRequest extends ReplicaRequest {
*/
boolean full();
+ /**
+ * Get the transaction coordinator inconsistent ID.
+ *
+ * @return the transaction coordinator inconsistent ID
+ */
+ String txCoordinatorId();
Review Comment:
Done
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -248,6 +254,10 @@ public void name(String newName) {
this.tableName = newName;
}
+ private String txCoordinatorId() {
Review Comment:
Done
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "Cyrill (via GitHub)" <gi...@apache.org>.
Cyrill commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1475773736
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/message/TxFinishReplicaRequest.java:
##########
@@ -59,6 +59,13 @@ public interface TxFinishReplicaRequest extends PrimaryReplicaRequest, Timestamp
*/
long commitTimestampLong();
+ /**
+ * Get the transaction coordinator inconsistent ID.
+ *
+ * @return the transaction coordinator inconsistent ID
+ */
+ String txCoordinatorId();
Review Comment:
See `PartitionListener.handleFinishTxCommand`, line 351:
```
markFinished(txId, cmd.commit(), cmd.commitTimestamp(), cmd.txCoordinatorId());
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1476052090
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -69,8 +69,8 @@ public class ReadWriteTransactionImpl extends IgniteAbstractTransactionImpl {
* @param observableTsTracker Observable timestamp tracker.
* @param id The id.
*/
- public ReadWriteTransactionImpl(TxManager txManager, HybridTimestampTracker observableTsTracker, UUID id) {
- super(txManager, id);
+ public ReadWriteTransactionImpl(TxManager txManager, HybridTimestampTracker observableTsTracker, UUID id, String txCoordinatorId) {
Review Comment:
Here and there, javadoc lacks txCoordinatorId param description. I'd also use one param at at row approach.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21371 Add tx coordinator id to TX requests [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3128:
URL: https://github.com/apache/ignite-3/pull/3128#discussion_r1474468571
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replication/request/CommittableTxRequest.java:
##########
@@ -32,6 +32,13 @@ public interface CommittableTxRequest extends ReplicaRequest {
*/
boolean full();
+ /**
+ * Get the transaction coordinator inconsistent ID.
+ *
+ * @return the transaction coordinator inconsistent ID
+ */
+ String txCoordinatorId();
Review Comment:
I'd say that it should be a field in ReadWriteReplicaRequest instead of CommittableTxRequest.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org