feat: Add support for server supported endpoints in config endpoint #1961
Conversation
amogh-jahagirdar
force-pushed
the
server-supported-endpoints
branch
from
36792b0 to
5a54643
Compare
| /// Check if a table exists in the catalog. | ||
| async fn table_exists(&self, table: &TableIdent) -> Result<bool> { | ||
| let context = self.context().await?; | ||
| Endpoint::check_supported(&context.config.endpoints, Endpoint::v1_table_exists())?; |
There was a problem hiding this comment.
This may break compatibility with older implementations where table existence checks happened through LoadTable.
There was a problem hiding this comment.
Yeah that's why I left this in draft, I wanted to think through more how to handle this particular case. We have a choice here, either we say that the rust client needs to work with those older servers so we take the complexity of not doing this strict check, or we opt for a cleaner solution which has some stricter assumptions.
I don't think it's too much extra work to handle the compatibility so I'd probably just address it.
| | Method::DELETE | ||
| | Method::HEAD | ||
| | Method::OPTIONS | ||
| | Method::PATCH => {} |
There was a problem hiding this comment.
Should we scope this down to the methods we actually handle?
There was a problem hiding this comment.
We could, I'm not opposed to that but I also think if someone adds the usage of these HTTP methods to the spec later, it creates another point in the client code that needs to be updated at that point, instead of "just working".
I figured since the REST spec is just defined from HTTP standard verbs, just allow all of the standard ones.
| fn default_endpoints() -> &'static HashSet<Endpoint> { | ||
| DEFAULT_ENDPOINTS.get_or_init(|| { | ||
| [ | ||
| Endpoint::v1_config(), |
There was a problem hiding this comment.
nit: we can remove config from this list
There was a problem hiding this comment.
Yes! good point, it's implicit that config is supported.
| eprintln!( | ||
| "DEBUG: endpoints value in load_table: {:?}", | ||
| context.config.endpoints | ||
| ); | ||
| eprintln!( | ||
| "DEBUG: looking for endpoint: {:?}", | ||
| Endpoint::v1_load_table() | ||
| ); |
There was a problem hiding this comment.
Need to clean this up
2 participants
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?