jerryshao commented on code in PR #14124:
URL: https://github.com/apache/gravitino/pull/14124#discussion_r4345721642
##########
docs/filesets.md:
##########
@@ -54,14 +55,17 @@ See [How to use GVFS](./how-to-use-gvfs.md).
### Managed and External
-A managed fileset belongs to Gravitino. Creating it creates the directory, and
deleting it deletes
-the data.
+A managed fileset belongs to Gravitino. Creating it creates the directory if
needed, and deleting
+it deletes the data.
An external fileset points at a location that already exists and stays under
someone else's control.
Review Comment:
The quick-start correction is right: an existing path does not make a
fileset external. The following sentence still says an external fileset points
at a location that already exists. Create `mkdirs` for both types when the path
is missing and adopts an existing directory for both. Please describe type as
an explicit lifecycle flag, not as "directory already existed."
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -1299,6 +1300,8 @@ private FilesetEntity updateFilesetEntity(
newName = ((FilesetChange.RenameFileset) change).getNewName();
} else if (change instanceof FilesetChange.UpdateFilesetComment) {
newComment = ((FilesetChange.UpdateFilesetComment)
change).getNewComment();
+ } else if (change instanceof FilesetChange.UpdateFilesetType) {
Review Comment:
`updateType` is authorized as a normal `alterFileset` (`FILESET::OWNER` or
`ANY_WRITE_FILESET`, see `FilesetOperations.java:290`), while drop is
owner-only. A principal with write but not drop can convert `EXTERNAL` to
`MANAGED` and thereby make a later owner drop or schema cascade recursively
delete every storage location. That is a new way for write to cause data loss.
Please require the drop privilege for `EXTERNAL` to `MANAGED`, or document and
test that write is intentionally enough to accept deletion.
##########
docs/manage-fileset-metadata-using-gravitino.md:
##########
@@ -183,6 +184,50 @@ Fileset fileset = filesets.alterFileset(
</TabItem>
</Tabs>
+#### Change the Fileset Type
+
+An existing fileset can be changed between `MANAGED` and `EXTERNAL`. This
changes metadata only:
+its identifier, name, storage locations, properties, and existing files are
preserved. Setting the
+current type is allowed.
+
+Change a `MANAGED` fileset to `EXTERNAL` before dropping it to retain its
files. Changing an
Review Comment:
Conversion to `MANAGED` deletes locations on drop only when filesystem ops
are enabled. With `disable-filesystem-ops=true`, drop still removes only
metadata. Worth one sentence next to the new lifecycle text so "accepts
deletion" is not read as unconditional.
##########
clients/client-python/gravitino/api/file/fileset_change.py:
##########
@@ -191,6 +212,26 @@ def __str__(self):
"""
return f"UPDATEFILESETCOMMENT {self._new_comment}"
+ @dataclass
+ class UpdateFilesetType:
+ """A fileset change to update the type without changing data or
locations."""
+
+ _new_type: Fileset.Type = field(metadata=config(field_name="newType"))
Review Comment:
nit: `UpdateFilesetType` works, but please match the other change classes:
snake_case field metadata on the change object (keep `newType` on the request
DTO) and an explicit `__eq__`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
