-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#318] refactor(core, catalog-*): Refactor the catalog operations to guarantee SSOT #403
Conversation
Code Coverage Report
Files
|
core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java
Show resolved
Hide resolved
core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java
Show resolved
Hide resolved
I suggest we come up with a systematic solution to log error operations in graviton and use it to make some modifications in graviton. As this PR allows failed actions in graviton and only log in |
23f6f60
to
301b552
Compare
identifier -> store.get(identifier, TABLE, TableEntity.class), | ||
"GET", | ||
stringId.id(), | ||
true /* throwIfNotFound */); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very clear: Why here is true
? As you ignore the failure when store TableEntity
to Graviton store in method CreateTable
, Chances are that we won't get TableEntity
in Graviton store, so here should be false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, for load operation, the behavior is that if we cannot get an entity from graviton store, we fail the operation rather than giving the user a half-complete metadata object.
My thinking of why we fail the operation rather than giving the user a half-complete metadata object is that: for load operation, we could fail without side-effect, so we deliver the user a consistent behavior; but for other operations that have side effects, if hive operation (for example) is succeed, we should not fail the operation to keep SSOT, otherwise it will be misleading and inconsistent (where hive operation succeeds but entity store operation fails).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment /* throwIfNotFound */
here is inconsistent with the actual behavior since NoSuchEntityException
was caught in operateOnEntity
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, for load operation, the behavior is that if we cannot get an entity from graviton store, we fail the operation rather than giving the user a half-complete metadata object.
@jerryshao I know your mean.
But I have a concern, for example,
1、In the CreateTable(tab1)
API, operation hive success, operation graviton's backend storage failed, The Graviton returns success.
2、 The user call loadTable(tab)
API return fails.
In this case, the User saves data successfully, and load data fails. and We can't help users to fix this problem, because we also use this loadTable()
API, and we also fail.
I think maybe we can return success and tell the user loss information of the graviton's backend storage in the REST Response,
In this way, the user can call alertTable()
API to refill lost information to fix this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, let me change the code.
core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java
Outdated
Show resolved
Hide resolved
ad01aff
to
f076e33
Compare
c050c00
to
4228339
Compare
core/src/main/java/com/datastrato/graviton/catalog/CatalogOperationDispatcher.java
Show resolved
Hide resolved
core/src/main/java/com/datastrato/graviton/catalog/rel/BaseSchema.java
Outdated
Show resolved
Hide resolved
I think maybe we need to add some test cases to cover Graviton's backend storage failures. |
For this multi-step operation, I think we needs to provide a queue computing function.
Use funQueueComputing() in the loadTable().
|
Let me think a bit on how to address the thing, but maybe we should not block on this. |
@xunliu can you please review it again, I would suggest not blocking the improvements you mentioned above. Several other PRs depend on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What changes were proposed in this pull request?
This is the final work of #250 , with this PR there're several major refactorings:
Why are the changes needed?
With this PR, we have several advantages:
Fix: #318
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Adding new UTs to cover the code