-
Notifications
You must be signed in to change notification settings - Fork 140
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
Nessie: Generic information for operations and content results #6616
Conversation
@dimas-b @adutra : I could need your opinion on this one. This PR is meant to have "something" in the REST v2 APi that allows us to send "additional information" or "metadata" along with a put-operation. A Nessie server would chose on its own what exactly it does does with each individual metadata object - it could only pass it "through" to Nessie events, or store it, or something else:tm:. Whether and how metadata could be returned as part of a "get-content(s)" can be defined later. Primary purpose of this PR is to have it defined in REST API v2 before we finalize it. |
api/model/src/main/java/org/projectnessie/model/ContentMetadata.java
Outdated
Show resolved
Hide resolved
*/ | ||
@JsonInclude(Include.NON_EMPTY) | ||
@JsonView(Views.V2.class) | ||
List<ContentMetadata> getMetadata(); |
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.
Should we perhaps have a new FetchOption
constant to control the loading of metadata in commit log (for example)?
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 current plan is to not store these metadata pieces in Nessie, but only to pass them "through" via Nessie events.
I've added the "response pieces" just in case we do wanna store those, but it definitely looks like that no part of that metadata needs to be persisted.
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.
Yes, but so that we can have a stable v2 API, shouldn't we also prepare new FetchOption
constants (or other means) to allow limiting the about of (unneeded) data in API responses?
However, #6634 will probably allow adding those enum constants later.
/** Additional content related information, if any. */ | ||
@JsonInclude(Include.NON_EMPTY) | ||
@JsonView(Views.V2.class) | ||
List<ContentMetadata> getMetadata(); |
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.
Could it be useful to have a list of metadata types in the corresponding request object?
Is it meaningful for a client to fetch (some) metadata without a Content
object?
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.
Not sure whether the getMetadata()
in ContentResponse
will be of any use short-term, but might be mid-term. Just would like to be prepared for the unlikely case.
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.
Well, should we support fetching content without the metadata (overhead) then?
public interface GenericMetadata { | ||
@JsonSerialize(as = ImmutableContentMetadata.class) | ||
@JsonDeserialize(as = ImmutableContentMetadata.class) | ||
public interface ContentMetadata { |
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.
Why do we have custom server-side serializers for Content
, but not for ContentMetadata
?
I guess it makes sense to have types java metadata wrappers the same way we have IcebergTable
et al.
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.
Conversely, if plain Json is ok for metadata, why not for Content
?
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'm actually thinking about whether it makes sense for Content
. At least clients could then receive unknown content-types without crashing during deserialization.
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 overall 👍
...odel/src/main/java/org/projectnessie/model/metadata/GenericContentMetadataSerialization.java
Outdated
Show resolved
Hide resolved
d47bdea
to
8a8008f
Compare
Removed the attributes in the get-content-responses |
Adds `List<ContentMetadata>` to `Operation.Put`, `ContentResponse` and its multiple-get conterpart. "Metadata" can be a lot of different things. If and how a Nessie server handles a particular "metadata variant" (think: type) depends on the Nessie server (configuration) and of course the variant itself. For example, one metadata variant might contain the Iceberg snapshot summary to be passed through via Nessie events, or passed through and also stored in Nessie. Removes the already unused `GenericMetadata` and its unused usages. Fixes projectnessie#6593
bit more boilerplate code, but not too bad
Adds
List<ContentMetadata>
toOperation.Put
,ContentResponse
and its multiple-get conterpart."Metadata" can be a lot of different things. If and how a Nessie server handles a particular "metadata variant" (think: type) depends on the Nessie server (configuration) and of course the variant itself.
For example, one metadata variant might contain the Iceberg snapshot summary to be passed through via Nessie events, or passed through and also stored in Nessie.
Removes the already unused
GenericMetadata
and its unused usages.Fixes #6593