Add NotificationType.VALIDATE which can serve as a dry-run of a CREATE without a metadata file #321
Conversation
…E without a metadata file If a remote catalog or manual caller wants to ensure that permissions, paths, etc., are configured correctly to receive CREATE/UPDATE notifications before deciding to actually create a table in the remote catalog, sending a VALIDATE notification with the prospective table metadata path can pre-validate basic setup. In a VALIDATE call, no actual entities will be mutated or created.
dennishuo
requested review from
eric-maynard,
collado-mike and
ebyhr
as code owners
| } | ||
|
|
||
| // Validate location against the resolvedStorageEntity | ||
| String metadataLocation = |
There was a problem hiding this comment.
To confirm, is the expectation is to supply a fake metadata.json location in the payload?
There was a problem hiding this comment.
I described some of the options in the rest spec description, but in a nutshell it can be either a parent directory or a prospective full filename, and the behavior is simply for the validation to ensure that whatever path is specified conforms to the ALLOWED_LOCATIONS of the storage config.
| // to findStorageInfoFromHierarchy. | ||
| PolarisResolvedPathWrapper resolvedStorageEntity = null; | ||
| Optional<PolarisEntity> storageInfoEntity = Optional.empty(); | ||
| for (int i = tableIdentifier.namespace().length(); i >= 0; i--) { |
There was a problem hiding this comment.
Do we still validate that this principle has privileges to create namespaces and table (if they don't exist)?
There was a problem hiding this comment.
Correct. This is validated in the updated PolarisCatalogHandlerWrapperAuthzTest.java
| } | ||
|
|
||
| @Test | ||
| public void testValidateNotificationFailToCreateFileIO() { |
There was a problem hiding this comment.
Another test case to add would be if the role doesn't have privileges to create the namespace or table
There was a problem hiding this comment.
Authz tests are covered in PolarisCatalogHandlerWrapperAuthzTest.java
|
|
||
| // The location of the metadata JSON file specified in the create will be forbidden. | ||
| // For a VALIDATE call we can pass in the metadata/ prefix itself instead of a metadata JSON | ||
| // filename. |
dennishuo
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995 and
flyrain
as code owners
There was a problem hiding this comment.
It would be nice to have a general checkPrivileges API so that a user can validate any privilege they require. it doesn't cover everything that's needed here, but maybe it's possible that we could model a privilege check to contain the information needed to actually validate most of the requirements. e.g., a TableGrant currently contains the privilege and the namespace and table name. For default table locations, that's all you'd need to validate create/read/write privileges. For custom table locations, we'd need a different entity to validate the location.
Yeah good point - I definitely also considered something along the lines of a "dry-run" parameter for this instead that could be generalized to other API requests. In this case when I started implementing it that way it became clear that there are enough deviations for the use case that it would be a bit misleading and messy to treat it as a dry run. In particular, this use case seems to pop up more prominently due to the inherently "cooperative" nature of the intended federation use case of the Notifications API, where a remote catalog wants to pre-validate before creating any metadata files on their side. This also means the control flow isn't quite the same as e.g. injecting a no-op metastore for writes while exercising all reads the same as "real" calls. I agree it'll be nice to have some generalized prevalidation longer-term though. Another thing that makes me want to lean towards a more targeted initial scope here is that the Notification API is more actively experimental while we better align in the broader community on the concept of external tables (https://lists.apache.org/thread/zcv6qm9ysknrhfpg093qgnrkrolptcht) so as long as the intended use case is something we see long-term (the use case of prevalidation/checkPrivilege/etc) then it makes sense to iterate on a narrower implementation that could change easily and then we can apply learnings from the use case towards the generalization of the implementation. |
A The prospect of Though I suppose this notify API is apt to change anyway, so it probably doesn't hurt to add a new notification type now, even if it is a bit wonky |
|
Thanks @dennishuo , thanks for reviews @collado-mike @tzuan16 and @sfc-gh-mparmar! |
Description
If a remote catalog or manual caller wants to ensure that permissions, paths, etc., are configured correctly to receive CREATE/UPDATE notifications before deciding to actually create a table in the remote catalog, sending a VALIDATE notification with the prospective table metadata path can pre-validate basic setup. In a VALIDATE call, no actual entities will be mutated or created.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Please delete options that are not relevant.