[#3607] feat(core): Support to import the entities when loading entities #3623

qqqttt123 · 2024-05-29T11:07:04Z

What changes were proposed in this pull request?

Support to import the entities when loading entities

Why are the changes needed?

Fix: #3607

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add ut.

jerryshao · 2024-05-30T07:03:17Z

From my point, I think:

Maybe we can use "managed/unmanaged" to differentiate the entity, not "imported"? We'd better figure out a better property name.
I saw that you're using a unified object API to import all the entities. As we only need to import schema, table, topic, maybe we can add APIs to each interface, like "..../table/{table_name}/manage" to mange the table, WDYT?
Is it better to use a property to demonstrate whether the entity is managed or not, not a field, the pro is that it is more compatible, the con is that user may not be aware of this, WDYT?

jerqi · 2024-05-30T07:28:18Z

2. /table/{table_name}/mana

I think import is better than managed/unmanaged. I think registered and metaImported is an alternative. Because some tables are created by us, but they didn't have entity in our storage. It's not suitable to use managed. Managed table has other meanings. So I prefer not using it.
I use unified api to do batch operations. It will be convenient for user.
I think it's better to use field. Because the property will write to the underlying system, some system doesn't support the property like schema of PG. This property is very important, so I prefer using field instead of property.
I don't update the property id for unimported tables in the underlying system, Because I just want to simplify the cases. Maybe I can do it in the future. If we don't update the property id for unimported tables in the underlying system, we can't handle the cases when users rename underlying system tables directly.

lw-yang · 2024-05-31T10:41:30Z

I'm considering a point: is it necessary to expose the concept of import to users?

when enterprises integrating Gravitino , there will be two phases

first is the transitional state during migration
second is the final state after migration is complete.

During the transitional state of migration, users request cannot yet converge on Gravitino, so we need to periodically import newly added tables, schemas... from outside into Gravitino. but for users, they don't need to care whether the state is "imported" or not.

When the migration is complete, all request will go through Gravitino, and at this point, "import" is no longer necessary.

another thing, why don't we automatically check and insert metadata during the get operation? This way, users wouldn't have to periodically import data.

qqqttt123 · 2024-05-31T11:58:41Z

I ever had concern about the issue of the lock.
We use the read lock when we read a table. If we add the import operation, we need add a parent lock for it. But I find I can use the double check to reduce the influence of the lock.

qqqttt123 · 2024-06-06T06:15:23Z

@lw-yang @jerryshao Could you help me review this pull request?

qqqttt123 · 2024-06-11T13:17:33Z

All comments are addressed. @jerryshao @yuqi1129 Could you take an another look?

jerryshao

Can you please carefully polish your code, I feel like that you didn't carefully check the behavior of each return value and the corner case. This makes me think that you don't have UTs to cover your different cases.

jerryshao · 2024-06-11T21:19:25Z

core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java

@@ -23,6 +24,9 @@ public final class EntityCombinedSchema implements Schema {

  // Sets of properties that should be hidden from the user.
  private Set<String> hiddenProperties;
+  // If imported is true, it means that storage backend have stored the correct entity.


Add a blank line above.

jerryshao · 2024-06-11T21:29:24Z

core/src/main/java/com/datastrato/gravitino/catalog/OperationDispatcher.java

@@ -272,6 +273,15 @@ boolean isManagedEntity(NameIdentifier catalogIdent, Capability.Scope scope) {
        IllegalArgumentException.class);
  }

+  boolean isEntityExist(NameIdentifier ident, Entity.EntityType type) {


Do you need thing method to be package private, or protected is enough?

Package private is enough. It should keep consistent with other methods.

I mean should protected be enough?

I changed to protected. But other methods in this class are package public. I think this method should have the same visibility with other methods with the same class.

jerryshao · 2024-06-11T21:32:44Z

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java

-                  catalogIdentifier,
-                  HasPropertyMetadata::schemaPropertiesMetadata,
-                  schema.properties()));
+    if (schema == null || schema.imported()) {


The method definition here will throw a NoSuchSchemaException instead of returning null, you should follow this, the method definition doesn't return null, instead it throws an exception.

jerryshao · 2024-06-11T21:34:44Z

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java

-                  catalogIdentifier,
-                  HasPropertyMetadata::schemaPropertiesMetadata,
-                  schema.properties()));
+    if (schema == null || schema.imported()) {


Again, do you really check that the code above will return null or throw an exception?

jerryshao · 2024-06-11T21:37:52Z

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java

-                catalogIdentifier,
-                HasPropertyMetadata::schemaPropertiesMetadata,
-                schema.properties()));
+    return schema;


Why do you return a schema you got previously here, IIUC, should we return a schema after it is imported?

If schema is imported, we don't need to import it again, so we can return it directly.

No, what I mean is that you will have a new entity combined schema in importSchema(), you should return the new one, not the old one. If you don't understand what I'm talking about, just ping me offline.

Has this comment been resolved? I have the same question, I think here should return EntityCombinedSchema after import.

After digging into the code, the difference of schema before and after import is only about AuditInfo, and since the imported entity doesn't have the new AuditInfo, so schema is the same before and after import, no need to create a new one. It is also the same for table and topic. @mchades .

Got it. If there is no change in the auditInfo then it is acceptable not to use imported EntityCombinedSchema here.

jerryshao · 2024-06-11T21:41:57Z

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java

@@ -330,4 +303,102 @@ public boolean dropSchema(NameIdentifier ident, boolean cascade) throws NonEmpty
        ? droppedFromStore
        : droppedFromCatalog;
  }
+
+  private boolean importSchema(NameIdentifier identifier) {


You didn't leverage the return value of this method, so why do you need to define a return value?

OK, Maybe I should add some logs.

jerryshao · 2024-06-11T21:52:37Z

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java

+    }
+
+    StringIdentifier stringId = getStringIdFromProperties(schema.properties());
+    // Case 1: The schema is not created by Gravitino.


You didn't update as what @yuqi1129 mentioned here.

jerryshao · 2024-06-11T21:59:04Z

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java

+            ident,
+            identifier -> store.get(identifier, SCHEMA, SchemaEntity.class),
+            "GET",
+            stringId.id());


For entity store, it throws a NoSuchEntityException instead of returning null. So the code below is not valid.

operateOnEntity will catch NoSuchEntityException and return null.

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java

core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java

mchades · 2024-06-14T04:22:03Z

core/src/main/java/com/datastrato/gravitino/catalog/EntityCombinedSchema.java

@@ -24,6 +25,10 @@ public final class EntityCombinedSchema implements Schema {
  // Sets of properties that should be hidden from the user.
  private Set<String> hiddenProperties;


Perhaps we can handle this not in this PR, we may need a class named 'combinedEntity' to unify the handling of entities from different sources.

@mchades what is the meaning here, can you explain more?

I mean do we need to provide an abstract class CombinedEntity<T> for EntityCombinedSchema, EntityCombinedTable, and EntityCombinedTopic?

qqqttt123 · 2024-06-17T12:51:12Z

core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java

+      // Load the schema to make sure the schema is imported.
+      SchemaDispatcher schemaDispatcher = GravitinoEnv.getInstance().schemaDispatcher();
+      NameIdentifier schemaIdent = NameIdentifier.of(ident.namespace().levels());
+      schemaDispatcher.loadSchema(schemaIdent);


It will produce an extra event of loadSchema if we use the loadSchema instead of SchemaExists. Is it expected?

But using SchemaExists seems a bit weird...

qqqttt123

LGTM.

core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java

jerryshao · 2024-06-18T01:29:02Z

@yuqi1129 @mchades can you please help to review?

jerryshao · 2024-06-18T07:51:34Z

Please review again.

yuqi1129

LGTM

mchades · 2024-06-18T07:57:56Z

Overall LGTM

… entities (apache#3623) ### What changes were proposed in this pull request? Support to import the entities when loading entities ### Why are the changes needed? Fix: apache#3607 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Add ut. --------- Co-authored-by: Heng Qin <[email protected]> Co-authored-by: Rory <[email protected]> Co-authored-by: Jerry Shao <[email protected]>

qqqttt123 marked this pull request as draft May 29, 2024 11:09

jerqi force-pushed the ISSUE-3607 branch 7 times, most recently from 9587cea to 30b5053 Compare May 30, 2024 06:40

jerqi force-pushed the ISSUE-3607 branch from 30b5053 to 08a28fe Compare May 30, 2024 07:08

jerqi force-pushed the ISSUE-3607 branch 6 times, most recently from ebf6696 to 20f57e5 Compare June 5, 2024 03:13

qqqttt123 changed the title ~~[ISSUE-3607] Support to sync the entities~~ [#3607] feat: Support to import the entities when loading entities Jun 5, 2024

Heng Qin added 3 commits June 5, 2024 15:54

[ISSUE-3607] Support to sync the entities

99b1caf

revert interface

3dac56c

Avoid null pointer exception

d5ca7e7

jerqi force-pushed the ISSUE-3607 branch 2 times, most recently from 219641e to d0910a7 Compare June 5, 2024 15:38

check schema

c831685

jerqi force-pushed the ISSUE-3607 branch from d0910a7 to c831685 Compare June 6, 2024 05:28

qqqttt123 marked this pull request as ready for review June 6, 2024 06:13

qqqttt123 requested a review from jerryshao June 6, 2024 06:13

jerqi added 2 commits June 11, 2024 21:13

Address comments

b5d5cd2

Remove unused code

f39720a

qqqttt123 requested review from jerryshao and yuqi1129 June 11, 2024 13:16

jerqi force-pushed the ISSUE-3607 branch from b85e8c1 to f39720a Compare June 11, 2024 16:31

jerryshao reviewed Jun 11, 2024

View reviewed changes

jerqi added 3 commits June 12, 2024 10:26

Address comments

08cf4e2

Add comment

e762b58

Address comments

ad5400a

mchades reviewed Jun 13, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/catalog/SchemaOperationDispatcher.java Outdated Show resolved Hide resolved

Address comments

e12c766

mchades reviewed Jun 14, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java Outdated Show resolved Hide resolved

mchades reviewed Jun 14, 2024

View reviewed changes

jerryshao added 2 commits June 17, 2024 20:35

Slightly refactor the code

bde6b0d

change code

870670e

qqqttt123 commented Jun 17, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/catalog/TableOperationDispatcher.java Outdated Show resolved Hide resolved

Polish the comment

8e2a7d3

jerryshao changed the title ~~[#3607] feat: Support to import the entities when loading entities~~ [#3607] feat(core): Support to import the entities when loading entities Jun 18, 2024

Address the comment

e661e56

yuqi1129 approved these changes Jun 18, 2024

View reviewed changes

jerryshao assigned qqqttt123 Jun 18, 2024

jerryshao merged commit c0f8be0 into apache:main Jun 18, 2024
33 checks passed

jerqi deleted the ISSUE-3607 branch June 18, 2024 11:44

		@@ -24,6 +25,10 @@ public final class EntityCombinedSchema implements Schema {
		// Sets of properties that should be hidden from the user.
		private Set<String> hiddenProperties;

[#3607] feat(core): Support to import the entities when loading entities #3623

[#3607] feat(core): Support to import the entities when loading entities #3623

Conversation

qqqttt123 commented May 29, 2024 • 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?

jerryshao commented May 30, 2024

jerqi commented May 30, 2024 • edited Loading

lw-yang commented May 31, 2024

qqqttt123 commented May 31, 2024

qqqttt123 commented Jun 6, 2024

qqqttt123 commented Jun 11, 2024

jerryshao left a 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

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

qqqttt123 Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

qqqttt123 left a comment

Choose a reason for hiding this comment

jerryshao commented Jun 18, 2024

jerryshao commented Jun 18, 2024

yuqi1129 left a comment

Choose a reason for hiding this comment

mchades commented Jun 18, 2024

qqqttt123 commented May 29, 2024 •

edited

Loading

jerqi commented May 30, 2024 •

edited

Loading

qqqttt123 Jun 17, 2024 •

edited

Loading