vinishjail97 commented on code in PR #917:
URL: https://github.com/apache/incubator-xtable/pull/917#discussion_r3908211422
##########
spec/rest-service-open-api.yaml:
##########
@@ -133,14 +134,22 @@ components:
source-table-path:
type: string
description: Path to the source table's metadata
+ source-data-path:
+ type: string
+ description: >-
+ Path to the source table's data files. Required: the service
passes it straight to
+ TargetTable.basePath, whose declaration is @NonNull, so omitting
it fails the request.
target-formats:
type: array
items:
type: string
description: List of desired output table formats (e.g., "ICEBERG",
"HUDI", "DELTA")
configurations:
type: object
- description: Additional configuration key/value pairs (e.g., storage
credentials or other service configs)
+ description: >-
+ Additional configuration key/value pairs. Only "partition-spec" is
read; every other key
+ is ignored. Credentials do not belong here -- the service reads
cloud storage with the
Review Comment:
Narrowing this is a clear improvement, and I agree with the security
instinct behind it. Telling people not to put credentials in a request body is
worth saying.
Would it work to keep the instruction and drop the justification, or
attribute it generally, roughly:
```yaml
description: >-
Additional configuration key/value pairs. Only "partition-spec"
is read; every other
key is ignored. Do not put storage credentials here. The service
reads cloud storage
with the credentials in its own environment, and request bodies
are not a safe place
for secrets in general.
```
##########
spec/rest-service-open-api.yaml:
##########
@@ -197,11 +206,6 @@ components:
minimum: 400
maximum: 600
description: HTTP response code
- stack:
Review Comment:
https://github.com/apache/incubator-xtable/blob/main/xtable-api/src/main/java/org/apache/xtable/model/sync/ErrorDetails.java
There is an ErrorDetails class that maps to ErrorModel? The stack trace is
something that can still be populated and useful part of the spec? Enhancing
xtable-service to populate this filed maybe a better fix?
##########
spec/rest-service-open-api.yaml:
##########
@@ -133,14 +134,22 @@ components:
source-table-path:
type: string
description: Path to the source table's metadata
+ source-data-path:
Review Comment:
`source-data-path` This is not a required field IMO and useful when data
files and metadata are located in different object storage prefixes?
--
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]
