[#5154] refactor(drop-metalake): re-define drop metalake #5155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR is based on #5067 |
mchades
force-pushed
the
drop-metalake
branch
13 times, most recently
from
aea5e9b to
53167c9
Compare
mchades
force-pushed
the
drop-metalake
branch
from
53167c9 to
9d106b1
Compare
jerryshao
reviewed
Oct 18, 2024
Comment on lines
190
to
195
| ErrorResponse resp = | ||
| restClient.get( | ||
| API_METALAKES_IDENTIFIER_PATH + name + "/activate", | ||
| ErrorResponse.class, | ||
| Collections.emptyMap(), | ||
| ErrorHandlers.metalakeErrorHandler()); |
There was a problem hiding this comment.
So you didn't update the code here for metalake, it should align with catalog to use patch with request body.
jerryshao
reviewed
Oct 18, 2024
| NameIdentifier metalakeIdent = NameIdentifier.of(ident.namespace().levels()); | ||
| checkMetalake(metalakeIdent, store); | ||
|
|
||
| if (!getInUseValue(store, ident)) { |
There was a problem hiding this comment.
It is better rename to getCatalogInUseValue for clear understanding.
jerryshao
reviewed
Oct 18, 2024
| @@ -116,14 +165,13 @@ public BaseMetalake createMetalake( | |||
| NameIdentifier ident, String comment, Map<String, String> properties) | |||
| throws MetalakeAlreadyExistsException { | |||
| long uid = idGenerator.nextId(); | |||
| StringIdentifier stringId = StringIdentifier.fromId(uid); | |||
There was a problem hiding this comment.
ince the uid was already in the metalake entity field and I don't see that uid being used in properties, I removed it.
jerryshao
reviewed
Oct 18, 2024
| boolean inUse = metalakeInUse(store, ident); | ||
| if (inUse && !force) { | ||
| throw new MetalakeInUseException( | ||
| "Metalake %s is in use, please deactivate it first or use force option", ident); |
There was a problem hiding this comment.
disable, not deactivate.
jerryshao
reviewed
Oct 18, 2024
Comment on lines
261
to
265
| // If reached here, it implies that the metalake is not in use or force is true. | ||
| if (inUse) { | ||
| // force is true, so deactivate the metalake first. | ||
| disableMetalake(ident); | ||
| } |
There was a problem hiding this comment.
The same here for metalake, we don't need to disable this before the drop.
jerryshao
reviewed
Oct 18, 2024
| @Produces("application/vnd.gravitino.v1+json") | ||
| @Timed(name = "activate-metalake." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) | ||
| @ResponseMetered(name = "activate-metalake", absolute = true) | ||
| public Response activateMetalake(@PathParam("name") String metalakeName) { |
There was a problem hiding this comment.
Why do we still keep this method and the below one?
jerryshao
reviewed
Oct 21, 2024
| * Otherwise, a {@link MetalakeInUseException} will be thrown. | ||
| * </ul> | ||
| * | ||
| * It is equivalent to calling {@code dropMetalake(ident, false)}. |
There was a problem hiding this comment.
{@code dropMetalake(name, false)}
|
Overall LGTM, just one small comment. |
jerryshao
approved these changes
Oct 21, 2024
mplmoknijb
pushed a commit
to mplmoknijb/gravitino
that referenced
this pull request
Nov 6, 2024
…e#5155) ### What changes were proposed in this pull request? - Add an in-use property to the metalake with the default value of true. - Only empty metalake with in-use=false can be dropped when `force=false` - User can use `dorce` option to drop metalake - More drop metalake limitations please see the JavaDoc of `dropMetalake` in API module ### Why are the changes needed? Fix: apache#5154 ### Does this PR introduce _any_ user-facing change? yes, users now can not drop an in used metalake ### How was this patch tested? tests added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
force=falsedorceoption to drop metalakedropMetalakein API moduleWhy are the changes needed?
Fix: #5154
Does this PR introduce any user-facing change?
yes, users now can not drop an in used metalake
How was this patch tested?
tests added