ibessonov commented on a change in pull request #7941:
URL: https://github.com/apache/ignite/pull/7941#discussion_r501659182
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/tree/io/PageIO.java
##########
@@ -258,6 +258,9 @@
/** */
public static final short T_MARKER_PAGE = 33;
+ /** */
+ public static final short T_INDEX_META = 34;
Review comment:
Can you please explain why you needed new IO type instead of new version
for existing type? You only add two fields, right? I don't get it then.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/tree/io/PageIO.java
##########
@@ -258,6 +258,9 @@
/** */
public static final short T_MARKER_PAGE = 33;
+ /** */
+ public static final short T_INDEX_META = 34;
Review comment:
I suggest class PageIndexIO being just T_META with version 2, see no
problems with this approach. Just as now, two different classes will add two
independent sets of new integers, that's fine. The only thing that's changed is
that PageIndexIO will have the same type as its base class.
Types should be immutable while pages are alive. This saves us from a lot of
troubles, I wouldn't abandon this tradition. That's exactly what versions are
for.
BTW your current code has bugs, you don't mark meta page as dirty in
"getOrAllocateCacheMetas" while upgrading, please fix it.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/tree/io/PageIO.java
##########
@@ -258,6 +258,9 @@
/** */
public static final short T_MARKER_PAGE = 33;
+ /** */
+ public static final short T_INDEX_META = 34;
Review comment:
I suggest class PageIndexIO being just T_META with version 2, see no
problems with this approach. Just as now, two different classes will add two
independent sets of new integers, that's fine. The only thing that's changed is
that PageIndexIO will have the same type as its base class.
Types should be immutable while pages are alive. This saves us from a lot of
troubles, I wouldn't abandon this tradition. That's exactly what versions are
for.
BTW your current code has bugs, you don't mark meta page as dirty in
"getOrAllocateCacheMetas" while upgrading, please fix it.
EDIT: I guess you can add getters to base class and implement them in
inheritors, while IOs that don't support them could just return 0 or throw
UnsupportedOperationException
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/tree/io/PageIO.java
##########
@@ -258,6 +258,9 @@
/** */
public static final short T_MARKER_PAGE = 33;
+ /** */
+ public static final short T_INDEX_META = 34;
Review comment:
Yes, this is correct. Naming of the classes doesn't bother me too much
so I was fine with "Indexing" instead of V2 (although that page is also
referred as "cache group meta page" sometimes, like in PageMemoryEx#metaPageId,
for example).
I think I understand the core problem, inheritance between MetaIO and
PartitionMetaIO should have been replaced with extension of the same abstract
superclass back in the day. But with current faulty design people just get
confused :(
----------------------------------------------------------------
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:
[email protected]
