Skip to content
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

Replace "property bags" in polaris-management-service.yml with specific attributes #555

Open
snazy opened this issue Dec 14, 2024 · 9 comments

Comments

@snazy
Copy link
Member

snazy commented Dec 14, 2024

Describe the bug

Most components in polaris-management-service.yml only have a generic property bag, but the valid keys/values are neither defined nor described anywhere.

Property bags are rather error-prone and can be accidentally/intentionally misused in many ways. A public facing API should define all attributes as specific properties with the correct types, including validation rules, descriptions and examples.

@collado-mike
Copy link
Contributor

There's a liberal use of the "bug" label for things that are personal preferences or, at best, generally good guidelines. We should stop using "bug" for things that aren't actually broken.

@snazy
Copy link
Member Author

snazy commented Jan 17, 2025

We can argue here whether it's an improvement or a bug. However, I definitely tend towards this being a bug, because the use of these property bags makes it extremely hard to distinguish which properties can be updated, whether changes are valid/authorized - a proper type model makes the meaning of the attributes clear for users and implementations.

@collado-mike
Copy link
Contributor

By that logic, any software written in an untyped language would be considered a bug.
I like types. I think more specific types are good.

I also think extension points are necessary and it's hard to account for all possible property types. An explicit choice about where to add extension points isn't a bug. It's certainly open for debate. I'm happy to collaborate on improvement proposals. But it's not a bug

@snazy
Copy link
Member Author

snazy commented Jan 17, 2025

That's good to hear. I'm not opposed of having extension points. But the fundamental and well defined things should really be typed components.

(Unrelated PS: I'm definitely not a fan of untyped languages - type related hidden and hard to detect bugs are a natural consequence of these langauges)

@collado-mike
Copy link
Contributor

(Unrelated PS: I'm definitely not a fan of untyped languages - type related hidden and hard to detect bugs are a natural consequence of these langauges)

I'm generally in agreement, but it's what I consider a "religious" discussion. I don't try to convert Baptists into Methodists and I generally stay away from "typed vs untyped" discussions. I care more about software quality and delivering value to users than I do about whether bugs are caught by the compiler or the tests.

@snazy
Copy link
Member Author

snazy commented Jan 17, 2025

I care more about software quality and delivering value

That's exactly the point of this issue. I think we're in agreement that proper typing is a big step towards better software quality.

@dimas-b
Copy link
Contributor

dimas-b commented Jan 17, 2025

From my POV this is not about java classes as types per se, but about using well-defined forms of configuration. Untyped languages can support that too (also, one can write object-oriented code in C ;) )

This is the point I also brought in #724 (comment)

@dennishuo
Copy link
Contributor

Yeah I hate untyped languages as well. My understanding is that generally the properties fields are all defined in Polaris entities simply for consistency to mirror the Iceberg REST entities defined in the core Iceberg REST spec. So the question would be whether we want the entity model to diverge from Iceberg.

All the internalProperties contained within the Polaris persistence layer are precisely funneled through Java wrappers like CatalogEntity, PrincipalEntity, etc., precisely to enforce typed attributes.

Is this suggestion in this issue to simply get rid of properties? As far as I recall there's only one case where there's a member of properties that is partially implicit, which I agree leads to some confusing parsing logic -- the default-base-location in Catalog is portrayed as a required string key in properties, whereas the look-and-feel of all other attributes would've made it fit better alongside e.g. type and storageConfigInfo as top-level attributes in the REST API model (and as a consequence, reside in internalProperties and be unpacked through CatalogEntity instead of residing in the public properties map).

@collado-mike collado-mike removed the bug Something isn't working label Jan 22, 2025
@collado-mike
Copy link
Contributor

I don't think we can get rid of the properties field, given that Iceberg uses it liberally. internalProperties is up for debate, though it needs to continue to be extensible - e.g., federated authn may need to set key/value pairs on the PrincipalEntity where neither the keys nor values are known by the Polaris project.

I removed the bug label, as while the goal is to "improve software quality", there is no active bug reported in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants