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/07/25 06:16:39 UTC
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #947: IGNITE-17372 Implement DeltaFilePageStore
tkalkirill commented on code in PR #947:
URL: https://github.com/apache/ignite-3/pull/947#discussion_r928504324
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaManager.java:
##########
@@ -95,10 +95,8 @@ public PartitionMeta readOrCreateMeta(
long partitionMetaPageId = partitionMetaPageId(groupPartitionId.getPartitionId());
try {
- if (filePageStore.size() > filePageStore.headerSize()) {
+ if (filePageStore.readWithoutPageIdCheck(partitionMetaPageId, buffer, false)) {
Review Comment:
Perhaps ideally the code should have looked something like this:
`
if (filePageStore.contains(partitionMetaPageId)) {
filePageStore.read(partitionMetaPageId, buffer, false);
return new PartitionMeta(checkpointId, ioRegistry.resolve(bufferAddr), bufferAddr);
} else {
PartitionMetaIo io = ...;
filePageStore.write(partitionMetaPageId, buffer.rewind(), true);
filePageStore.sync();
return new PartitionMeta(checkpointId, io, bufferAddr);
}
`
But it will always go on a false branch, because **contains** will have to check it by the number of pages, which we can only find out about by reading the first page, I don’t want to write the logic for **PartitionMetaIo** inside, I think this is not correct.
We can personally discuss the solution and come up with a better API, but so far it seems to be more compromise.
##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/persistence/PartitionMetaManager.java:
##########
@@ -95,10 +95,8 @@ public PartitionMeta readOrCreateMeta(
long partitionMetaPageId = partitionMetaPageId(groupPartitionId.getPartitionId());
try {
- if (filePageStore.size() > filePageStore.headerSize()) {
+ if (filePageStore.readWithoutPageIdCheck(partitionMetaPageId, buffer, false)) {
Review Comment:
Perhaps ideally the code should have looked something like this:
`
if (filePageStore.contains(partitionMetaPageId)) {
filePageStore.read(partitionMetaPageId, buffer, false);
return new PartitionMeta(checkpointId, ioRegistry.resolve(bufferAddr), bufferAddr);
} else {
PartitionMetaIo io = ...;
filePageStore.write(partitionMetaPageId, buffer.rewind(), true);
filePageStore.sync();
return new PartitionMeta(checkpointId, io, bufferAddr);
}
`
But it will always go on a false branch, because **contains** will have to check it by the number of pages, which we can only find out about by reading the first page, I don’t want to write the logic for **PartitionMetaIo** inside, I think this is not correct.
We can personally discuss the solution and come up with a better API, but so far it seems to be more compromise.
--
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