[#305][#306] improvement(core): Adjust id name mapping to support true deletion and modification of entities #309

yuqi1129 · 2023-09-01T06:08:27Z

What changes were proposed in this pull request?

We change the key of name-id mapping pairs.

Why are the changes needed?

Current name-id mapping contains no namespace information and can hardly handle the following scenarios:

Rename an entity when there exists an entity with the same name under a different namespace.
Delete entities recursively.

Fix: #305 #306

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UTs can cover this change.

github-actions · 2023-09-01T06:10:14Z

Code Coverage Report

Overall Project	63.02% `-0.17%`	🟢
Files changed	94.27%	🟢

Module	Coverage
api	71.54% `-0.34%`	🔴
core	71.43% `-0.42%`	🟢
catalog-hive	58.43%	🟢

Files

Module	File	Coverage
api	NonEmptyEntityException.java	0%	🔴
core	EntityKeyEncoder.java	100%	🟢
	EntityStore.java	100%	🟢
	BinaryEntityKeyEncoder.java	99.47%	🟢
	KvEntityStore.java	93.78% `-1.28%`	🟢
	BaseTable.java	79.52% `-1.51%`	🟢
	BaseSchema.java	79.23% `-1.6%`	🟢
	RocksDBKvBackend.java	68.63% `-3.99%`	🟢
	KvBackend.java	0%	🟢
catalog-hive	HiveCatalogOperations.java	66.17%	🟢

jerryshao · 2023-09-01T06:57:30Z

You'd better add more tests to cover different scenarios.

api/src/main/java/com/datastrato/graviton/exceptions/SubEntitiesNoEmptyException.java

core/src/main/java/com/datastrato/graviton/storage/EntityKeyEncoder.java

jerryshao · 2023-09-04T07:24:46Z

core/src/main/java/com/datastrato/graviton/storage/EntityKeyEncoder.java

+   * @return
+   * @throws IOException
+   */
+  String generateIdNameMappingKey(NameIdentifier nameIdentifier) throws IOException;


should it be exposed to the outside uers?

generateIdNameMappingKey was called by KvEntityStore temporarily. So what you mean outside users?

I mean other classes that use this EntityKeyEncoder.

Basically, I think exposing this method looks a little odd to me, as this seems should not be known by the users.

Besides, the name of this method and below are confused.

OK, Let me see

I was going to move this to NameMappingService interface, but it's a method that's strongly associated with kv, and it didn't seem right to move it to that interface.

So I also move it to KvEntityStore, can you give some advice?

jerryshao · 2023-09-04T07:26:32Z

core/src/main/java/com/datastrato/graviton/storage/EntityKeyEncoder.java

+   * @return
+   * @throws IOException
+   */
+  List<T> encodeSubEntityPrefix(NameIdentifier identifier, EntityType type) throws IOException;


should it be exposed to the outside users?

basically, what's the meaning of this method, the doc written here is confused.

basically, what's the meaning of this method, the doc written here is confused.

I want to get the prefix of all sub-entities of an entities with the name identifier . E.g., Assuming the id of a metalake is 1, then prefix of all sub-entities (catalog, schema, table) are:

ca_{metalake_id} is the prefix a catalog entity in metalake 1
sc_{metalake_id} is the prefix a schema entity in metalake 1
ta_{metalake_id} is the prefix a table entity in metalake 1

should it be exposed to the outside users?

KvEntityStore needs this function to remove all sub-entities

Can we redesign the interface to not expose so many details to the classes who use this.

Method encodeSubEntityPrefix seems to be closely related with KV Implementation(Get all prefix), So I move this to KvEntityStore

core/src/main/java/com/datastrato/graviton/storage/kv/BinaryEntityKeyEncoder.java

jerryshao · 2023-09-04T07:41:57Z

core/src/main/java/com/datastrato/graviton/storage/kv/KvEntityStore.java

+    if (subEntityPrefix.isEmpty()) {
+      // has no sub-entities
+      return backend.delete(dataKey);
+    }


Why don't you delete name-id mappings also?

I think we do not need to remove name-id mapping eagerly for the following reasons:

name-id mappings can be reused later

name-id mappings can be deleted offline for better performance

catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java

core/src/main/java/com/datastrato/graviton/proto/TableEntitySerde.java

jerryshao · 2023-09-05T02:04:25Z

core/src/main/java/com/datastrato/graviton/storage/kv/BinaryEntityKeyEncoder.java

   */
-  private byte[] encodeEntity(NameIdentifier identifier, EntityType entityType) throws IOException {
+  private byte[] encodeEntity(
+      NameIdentifier identifier, EntityType entityType, boolean returnNullIfEntityNotFound)


We'd also change the name "returnNullIfEntityNotFound" to be short

jerryshao · 2023-09-05T05:58:48Z

core/src/main/java/com/datastrato/graviton/storage/kv/KvEntityStore.java

+              generateIdNameMappingKey(ident), generateIdNameMappingKey(updatedE.nameIdentifier()));
+
+          // Update the entity to store
+          backend.put(key, serDe.serialize(updatedE), true);


Why don't we directly call put?

The scenario is changing the name of entity and the key already exists, so we should use overwrite option. If we use put without overwrite, a AlreadyExistsException will be thrown.

jerryshao · 2023-09-05T06:07:38Z

core/src/main/java/com/datastrato/graviton/storage/kv/KvEntityStore.java

+    return StringUtils.isBlank(context) ? name : context + NAMESPACE_SEPARATOR + name;
+  }
+
+  public String generateIdNameMappingKey(NameIdentifier nameIdentifier) throws IOException {


generateIdNameMappingKey and generateKeyForNameMapping these two names are a bit long and confused, can you simplify the name?

jerryshao · 2023-09-05T06:09:29Z

You need to update the rfc-2 markdown file also.

support issue 305

dbea460

jerryshao changed the title ~~[#305] improvement: Adjust id name mapping to support true deletion and modification of entities~~ [#305] improvement(core): Adjust id name mapping to support true deletion and modification of entities Sep 4, 2023

fix problems

035f5a2

yuqi1129 changed the title ~~[#305] improvement(core): Adjust id name mapping to support true deletion and modification of entities~~ [#305][#306] improvement(core): Adjust id name mapping to support true deletion and modification of entities Sep 4, 2023

yuqi1129 requested review from jerryshao and mchades September 4, 2023 06:50

yuqi1129 self-assigned this Sep 4, 2023

yuqi1129 added 4 commits September 4, 2023 15:14

revert some code

954c93a

Add more test case

9b3b139

Fix format

fd637b2

Minor changes;

363a0e7

jerryshao reviewed Sep 4, 2023

View reviewed changes

yuqi1129 added 4 commits September 4, 2023 16:08

comment 1

491e6ba

comment 1

b0dd13a

Fix format

89236b5

Fix format again

2c5c632

mchades reviewed Sep 4, 2023

View reviewed changes

catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java Show resolved Hide resolved

mchades reviewed Sep 4, 2023

View reviewed changes

core/src/main/java/com/datastrato/graviton/proto/TableEntitySerde.java Outdated Show resolved Hide resolved

yuqi1129 added 2 commits September 4, 2023 19:53

Fix comment again

97cd277

Refactor interface

d3c6f4d

jerryshao reviewed Sep 5, 2023

View reviewed changes

yuqi1129 added 2 commits September 5, 2023 15:02

Fix comments about rename and changes in doc

29ddd26

Increase UT coverage rate

a5658c0

jerryshao approved these changes Sep 5, 2023

View reviewed changes

jerryshao merged commit 44d041e into apache:main Sep 5, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#305][#306] improvement(core): Adjust id name mapping to support true deletion and modification of entities #309

[#305][#306] improvement(core): Adjust id name mapping to support true deletion and modification of entities #309

yuqi1129 commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading

jerryshao commented Sep 1, 2023

jerryshao Sep 4, 2023

yuqi1129 Sep 4, 2023

jerryshao Sep 4, 2023

yuqi1129 Sep 4, 2023

yuqi1129 Sep 4, 2023

jerryshao Sep 4, 2023

jerryshao Sep 4, 2023

yuqi1129 Sep 4, 2023

jerryshao Sep 4, 2023

yuqi1129 Sep 4, 2023

jerryshao Sep 4, 2023

yuqi1129 Sep 4, 2023

jerryshao Sep 5, 2023

yuqi1129 Sep 5, 2023

jerryshao Sep 5, 2023

yuqi1129 Sep 5, 2023

jerryshao Sep 5, 2023

yuqi1129 Sep 5, 2023

jerryshao commented Sep 5, 2023

[#305][#306] improvement(core): Adjust id name mapping to support true deletion and modification of entities #309

[#305][#306] improvement(core): Adjust id name mapping to support true deletion and modification of entities #309

Conversation

yuqi1129 commented Sep 1, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

github-actions bot commented Sep 1, 2023 • edited Loading

Code Coverage Report

jerryshao commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Sep 5, 2023

yuqi1129 commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading