You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/06/15 07:34:28 UTC
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #834: IGNITE-16881 Multi version storage was integrated. Transaction ID is a UUID now.
sanpwc commented on code in PR #834:
URL: https://github.com/apache/ignite-3/pull/834#discussion_r893481601
##########
examples/src/integrationTest/java/org/apache/ignite/example/storage/ItPageMemoryStorageExampleTest.java:
##########
@@ -23,11 +23,13 @@
import org.apache.ignite.example.AbstractExamplesTest;
import org.apache.ignite.internal.storage.pagememory.PageMemoryStorageEngine;
import org.apache.ignite.internal.storage.pagememory.configuration.schema.PageMemoryStorageEngineConfiguration;
+import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
/**
* For testing examples demonstrating work with {@link PageMemoryStorageEngine}.
*/
+@Disabled("https://issues.apache.org/jira/browse/IGNITE-16923")
Review Comment:
Do we really need whole epic to be implemented?
##########
modules/api/src/main/java/org/apache/ignite/configuration/schemas/table/TablesConfigurationSchema.java:
##########
@@ -37,5 +37,6 @@ public class TablesConfigurationSchema {
/** Default data storage for tables. */
@ExistingDataStorage
@Value(hasDefault = true)
- public String defaultDataStorage = "pagememory";
+ // TODO: IGNITE-16923 Set "pagememory" after the ticket is resolved.
Review Comment:
Same as above, do we really need whole epic to be implemented?
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java:
##########
@@ -114,6 +114,12 @@ public RowId insert(BinaryRow row, UUID txId) throws StorageException {
/** {@inheritDoc} */
@Override
public @Nullable BinaryRow abortWrite(RowId rowId) {
+ VersionChain chain = map.get(rowId);
Review Comment:
Could you please prove that it's thread-safe? In the origin version everything goes in the atomic computeIfPresent().
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java:
##########
@@ -154,6 +160,12 @@ private void cleanupIndexesForAbortedRow(VersionChain versionChain, RowId rowId)
/** {@inheritDoc} */
@Override
public void commitWrite(RowId rowId, Timestamp timestamp) {
+ VersionChain chain = map.get(rowId);
Review Comment:
Same as for abortWrite.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -335,7 +334,7 @@ private VersionChain findVersionChainForModification(RowId rowId) {
VersionChain currentVersionChain = findVersionChainForModification(rowId);
if (currentVersionChain.transactionId() == null) {
- throw new NoUncommittedVersionException();
Review Comment:
Why?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -213,8 +212,9 @@ private void writeUnversioned(byte[] keyArray, BinaryRow row, UUID txId) throws
try {
byte[] previousValue = db.get(cf, keyBuf.array(), 0, ROW_PREFIX_SIZE);
+
if (previousValue == null) {
- throw new NoUncommittedVersionException();
Review Comment:
Same as above.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -379,12 +380,18 @@ private void updateAssignmentInternal(ConfigurationNotificationEvent<byte[]> ass
InternalTable internalTable = tablesById.get(tblId).internalTable();
try {
+ // TODO: IGNITE-16923 Remove assert after the ticket is resolved.
+ assert internalTable.storage() instanceof MvTableStorage :
+ "Only multi version storages are supported.";
+
+ MvTableStorage storage = (MvTableStorage) internalTable.storage();
+
futures[partId] = raftMgr.updateRaftGroup(
raftGroupName(tblId, partId),
newPartitionAssignment,
toAdd,
() -> new PartitionListener(tblId,
- new VersionedRowStore(internalTable.storage().getOrCreatePartition(partId),
+ new VersionedRowStore(storage.createPartition(partId),
Review Comment:
Please preserve storage retrieval inlining here:
`(MvTableStorage) internalTable.storage()`
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItColocationTest.java:
##########
@@ -112,13 +111,13 @@ public class ItColocationTest {
TxManager txManager = new TxManagerImpl(clusterService, new HeapLockManager()) {
@Override
- public CompletableFuture<Void> finishRemote(NetworkAddress addr, Timestamp ts, boolean commit, Set<String> groups) {
+ public CompletableFuture<Void> finishRemote(NetworkAddress addr, UUID id, boolean commit, Set<String> groups) {
return CompletableFuture.completedFuture(null);
}
};
txManager.start();
- MessagingService messagingService = MessagingServiceTestUtils.mockMessagingService(txManager);
+ MessagingService messagingService = Mockito.mock(MessagingService.class, RETURNS_DEEP_STUBS);;
Review Comment:
Why?
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageTest.java:
##########
@@ -554,11 +554,17 @@ void commitWriteMakesVersionAvailableToReadByTimestamp() {
}
@Test
- void abortWriteFailsIfNoUncommittedVersionExists() {
+ void commitAndAbortWriteNoOpIfNoUncommittedVersionExists() {
RowId rowId = storage.insert(binaryRow, newTransactionId());
storage.commitWrite(rowId, Timestamp.nextVersion());
- assertThrows(NoUncommittedVersionException.class, () -> storage.abortWrite(rowId));
Review Comment:
If we'll stick with not throwing NoUncommittedVersionException from storages, we should entirely remove such an exception and adopt TestMvPartitionStorage.
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -237,7 +237,9 @@ public void commitWrite(RowId rowId, Timestamp timestamp) throws StorageExceptio
// Read a value associated with pending write.
byte[] valueBytes = db.get(cf, keyBuf.array(), 0, ROW_PREFIX_SIZE);
- assert valueBytes != null : "Failed to commit row " + rowId + ", value is missing";
+ if (valueBytes == null) {
Review Comment:
Same as above.
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbMvPartitionStorage.java:
##########
@@ -356,7 +358,7 @@ private Cursor<BinaryRow> scan(Predicate<BinaryRow> keyFilter, @Nullable Timesta
// - no one guarantees that there will only be a single cursor;
// - no one guarantees that returned cursor will not be used by other threads.
// The thing is, we need this buffer to preserve its content between invocactions of "hasNext" method.
- ByteBuffer seekKeyBuf = ByteBuffer.allocate(seekKeyBufSize).order(BIG_ENDIAN);
+ ByteBuffer seekKeyBuf = ByteBuffer.allocate(seekKeyBufSize).order(BIG_ENDIAN).putShort(0, (short) partitionId);
Review Comment:
Why? What's the point of initializing seekKeyBuf with 0 partitionId?
##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/basic/TestMvPartitionStorage.java:
##########
@@ -114,6 +114,12 @@ public RowId insert(BinaryRow row, UUID txId) throws StorageException {
/** {@inheritDoc} */
@Override
public @Nullable BinaryRow abortWrite(RowId rowId) {
+ VersionChain chain = map.get(rowId);
+
+ if (chain.begin != null && chain.txId == null) {
Review Comment:
I'd rather add a method boolean containsWriteIntent() or similar to VersisionedChain with corresponding explanation that it's only possible to have at most one writeIntent at the head of version chain with null begin timestamp and non-null txId that is used for write intent resolution.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/command/FinishTxCommand.java:
##########
@@ -17,35 +17,43 @@
package org.apache.ignite.internal.table.distributed.command;
-import org.apache.ignite.internal.tx.Timestamp;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import org.apache.ignite.lang.IgniteUuid;
import org.apache.ignite.raft.client.WriteCommand;
/** State machine command to finish a transaction. */
public class FinishTxCommand implements WriteCommand {
- /** The timestamp. */
- private final Timestamp timestamp;
+ /** Transaction id. */
+ private final UUID txId;
/** Commit or rollback state. */
private final boolean finish;
+ /** Keys that are locked by the transaction. */
+ private Map<IgniteUuid, List<byte[]>> lockedKeys;
+
/**
* The constructor.
*
- * @param timestamp The timestamp.
- * @param finish Commit or rollback state {@code True} to commit.
+ * @param txId The txId.
+ * @param finish Commit or rollback state {@code True} to commit.
+ * @param lockedKeys Keys that are locked by the transaction.
Review Comment:
Let's explicitly denote that IgniteUuid in lockedKeys is a lock id.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -379,12 +380,18 @@ private void updateAssignmentInternal(ConfigurationNotificationEvent<byte[]> ass
InternalTable internalTable = tablesById.get(tblId).internalTable();
try {
+ // TODO: IGNITE-16923 Remove assert after the ticket is resolved.
+ assert internalTable.storage() instanceof MvTableStorage :
+ "Only multi version storages are supported.";
+
+ MvTableStorage storage = (MvTableStorage) internalTable.storage();
+
futures[partId] = raftMgr.updateRaftGroup(
raftGroupName(tblId, partId),
newPartitionAssignment,
toAdd,
() -> new PartitionListener(tblId,
- new VersionedRowStore(internalTable.storage().getOrCreatePartition(partId),
+ new VersionedRowStore(storage.createPartition(partId),
Review Comment:
Is it valid to substitute getOrCreate with simple create?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PageMemoryMvPartitionStorage.java:
##########
@@ -371,7 +370,9 @@ public void commitWrite(RowId rowId, Timestamp timestamp) throws StorageExceptio
VersionChain currentVersionChain = findVersionChainForModification(rowId);
long chainLink = PartitionlessLinks.addPartitionIdToPartititionlessLink(currentVersionChain.headLink(), partitionId);
- assert currentVersionChain.transactionId() != null;
+ if (currentVersionChain.transactionId() == null) {
Review Comment:
Again, why? For all uncommited chains we should have intents on top with non null txIds.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableTxManagerImpl.java:
##########
@@ -48,8 +48,9 @@ public TableTxManagerImpl(ClusterService clusterService, LockManager lockManager
/** {@inheritDoc} */
@Override
- protected CompletableFuture<?> finish(String groupId, Timestamp ts, boolean commit) {
- ActionRequest req = FACTORY.actionRequest().command(new FinishTxCommand(ts, commit)).groupId(groupId).readOnlySafe(true).build();
+ protected CompletableFuture<?> finish(String groupId, UUID id, boolean commit) {
Review Comment:
I'd rather name it as txId like you've done in TxManagerImpl.
--
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