xtern commented on a change in pull request #7941:
URL: https://github.com/apache/ignite/pull/7941#discussion_r501722997
##########
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, we adding two int fields into two pages.
We have 2 types of memory pages:
T_META (index partition)
T_PART_META (other partition)
We want to add 2 ints into each of them, but don't forget that IO for
T_PART_META extends IO for T_META.
In my first approach I added 2 ints into T_META, and added
PagePartitionMetaIOV3 for T_PART_META.
But since PagePartitionMetaIO "extends" PageMetaIO all offsets in
PagePartitionMetaIO was shifted and there was a problem with inheritance (for
example in PagePartitionMetaIO/V2 new methods were visible, all offsets was
redefined in PagePartitionMetaIOV3), you can imagine what it looked like in
commit with the second approach
https://github.com/apache/ignite/pull/7941/commits/036b931bd40e2ffbd519779fde86fae7343da23e
With current approach we simply create the new IO that is only used for the
"index" partition.
If you can suggest a better approach how for storing 2 extra int's itn the
index meta page without losing binary compatibility, please describe.
p.s. I explained this change in wiki
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652384#TDE.Phase3.Cachekeyrotation.-PageMetaIOandPagePartitionMetaIO
##########
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, we adding two int fields into two pages.
We have 2 types of memory pages:
T_META (index partition)
T_PART_META (other partition)
We want to add 2 ints into each of them, but don't forget that IO for
T_PART_META extends IO for T_META.
In my first approach I added 2 ints into T_META, and added
PagePartitionMetaIOV3 for T_PART_META.
But since PagePartitionMetaIO "extends" PageMetaIO all offsets in
PagePartitionMetaIO was shifted and there was a problem with inheritance (for
example in PagePartitionMetaIO/V2 new methods were visible, all offsets was
redefined in PagePartitionMetaIOV3), you can imagine what it looked like in
commit with the second approach
https://github.com/apache/ignite/pull/7941/commits/036b931bd40e2ffbd519779fde86fae7343da23e
With current approach we simply create the new IO that is only used for the
"index" partition.
If you can suggest a better approach for storing 2 extra int's in the index
meta page without losing binary compatibility, please describe.
p.s. I explained this change in wiki
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652384#TDE.Phase3.Cachekeyrotation.-PageMetaIOandPagePartitionMetaIO
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/pagemem/wal/record/EncryptedRecord.java
##########
@@ -43,7 +43,7 @@ public EncryptedRecord(int grpId, RecordType plainRecType) {
/** {@inheritDoc} */
@Override public RecordType type() {
- return RecordType.ENCRYPTED_RECORD;
+ return RecordType.ENCRYPTED_RECORD_V2;
Review comment:
For binary compatibility.
For ENCRYPTED_RECORD_V2 we reading 1 extra byte (encryption key identifier).
(see change in RecordDataV1Serializer.readEncryptedData)
##########
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:
If I understood you correctly, you suggest to add PageMetaIOV2 (instead
of PageIndexMetaIO)?
PagePartitionMeta will continue to extend PageMetaIO(not v2), is this
correct?
##########
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:
If I understood you correctly, you suggest to add PageMetaIOV2 (instead
of PageIndexMetaIO) and keep page type T_META?
PagePartitionMeta will continue to extend PageMetaIO(not v2), is this
correct?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/pagemem/wal/record/EncryptedRecord.java
##########
@@ -43,7 +43,7 @@ public EncryptedRecord(int grpId, RecordType plainRecType) {
/** {@inheritDoc} */
@Override public RecordType type() {
- return RecordType.ENCRYPTED_RECORD;
+ return RecordType.ENCRYPTED_RECORD_V2;
Review comment:
Thanks for the suggestion, as far as I understand you are suggesting not
to modify any existing WAL record classes.
Could you please give an example of a compatibility issue that we may face
in the future (in theory)?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/pagemem/wal/record/EncryptedRecord.java
##########
@@ -43,7 +43,7 @@ public EncryptedRecord(int grpId, RecordType plainRecType) {
/** {@inheritDoc} */
@Override public RecordType type() {
- return RecordType.ENCRYPTED_RECORD;
+ return RecordType.ENCRYPTED_RECORD_V2;
Review comment:
Thanks for the suggestion, as far as I understand you are suggesting not
to modify any existing WAL record classes.
Could you please give an example of a compatibility issue that we may face
in the future (in theory) that can be avoided by keeping the old "unused"
classes?
##########
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:
Thanks, I believe I've fixed it.
----------------------------------------------------------------
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]
