Skip to content

Conversation

@tkalkirill
Copy link
Contributor

* Abstract implementation of {@link PartitionStorage} based on a {@link BplusTree}.
*/
// TODO: IGNITE-16644 Support snapshots.
abstract class AbstractPageMemoryPartitionStorage implements PartitionStorage {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this class. PartitionStorage is deprecated, what's the point? Can you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will get rid of this in a separate IGNITE-17466, but for now, let it be right

@Override
public PageMemoryMvPartitionStorage createMvPartitionStorage(int partitionId) {
throw new UnsupportedOperationException("Not supported yet");
public PersistentPageMemoryMvPartitionStorage createMvPartitionStorage(int partitionId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an old method for creating partition? It can just throw UnsupportedOperationException.

Why did you rename partId to partitionId in method that has nothing to do with your code btw? Why was it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get rid of the old method in ticket IGNITE-17466, what's stopping you from renaming?

}

@Test
void testVersionChainTreeRootPageId() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these tests for? Just to test setters and getters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but also to check metadata snapshots for those setters


CheckpointManager checkpointManager = dataRegion.checkpointManager();

CheckpointTimeoutLock checkpointTimeoutLock = checkpointManager.checkpointTimeoutLock();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but I think that CheckpointTimeoutLock is a very misleading name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a ticket for fixing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know because we're trying to take the lock by timeout.

@ibessonov ibessonov merged commit 2a6d307 into apache:main Aug 5, 2022
@ibessonov ibessonov deleted the ignite-17085 branch August 5, 2022 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants