[core] Introduce properties field in Snapshot#5703
Conversation
luoyuxia
changed the title
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java
Outdated
Show resolved
Hide resolved
|
Left minor comments, please take a look and fix failed tests. |
| protected final String statistics; | ||
|
|
||
| // properties | ||
| // null for paimon <= 1.1 |
There was a problem hiding this comment.
What about empty? We don't want to include empty map.
There was a problem hiding this comment.
Now, I modified it to be null when empty map
| logOffsets.put(bucket, newOffset); | ||
| } | ||
|
|
||
| public void addProperty(String key, String value) { |
There was a problem hiding this comment.
Can you show the example? How to add property? Add what property.
There was a problem hiding this comment.
Maybe something like:
ManifestCommittable committable = new ManifestCommittable(COMMIT_IDENTIFIER);
committable. addProperty("fluss_offset_partition_1_bucket1", "14")
committable. addProperty("fluss_offset_partition_2_bucket2", "15")
9128e0e to
3a84f8d
Compare
|
@JingsongLi @yuzelin Thanks for reviewing. Comments has been addressed. |
| @@ -104,7 +104,7 @@ public void testCompatibilityToV3CommitV7() throws IOException { | |||
|
|
|||
| ManifestCommittableSerializer serializer = new ManifestCommittableSerializer(); | |||
There was a problem hiding this comment.
Add a new test for current version.
| ManifestCommittableSerializer serializer = new ManifestCommittableSerializer(); | ||
| byte[] bytes = serializer.serialize(manifestCommittable); | ||
| ManifestCommittable deserialized = serializer.deserialize(3, bytes); | ||
| ManifestCommittable deserialized = serializer.deserialize(serializer.getVersion(), bytes); |
There was a problem hiding this comment.
Please don't change this.
There was a problem hiding this comment.
After look in deep again, maybe we still need to change to use serializer.getVersion() which is v4 to deserialize it.
According to java doc, the version is for in which the data was serialized in here, the data is serialized with v4.
And in v4, we will serialize properties into the bytes, it deserialize with v3, it'll skip the properties bytes and try to deserialize it as commitMessages and throw BufferUnderflowException.
|
@JingsongLi Thanks for review. Add the test now. |
luoyuxia
force-pushed
the
add-properties-in-snapshot
branch
6 times, most recently
from
1bac840 to
287b1d2
Compare
luoyuxia
force-pushed
the
add-properties-in-snapshot
branch
from
287b1d2 to
5c1f4d4
Compare
(cherry picked from commit b13100d)
3 participants
Purpose
Linked issue: close #5649
Introduce properties field in Snapshot
Tests
testCommitManifestWithProperties
API and Format
Documentation