-
Notifications
You must be signed in to change notification settings - Fork 117
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
Helm chart: minor improvement to JDBC section in docs #8717
Conversation
site/docs/guides/kubernetes.md
Outdated
The database and schema can be either passed in the JDBC URL (recommended) or as separate | ||
configuration options. The following configuration is equivalent to the one above: |
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 believe for PosgreSQL both the JDBC URL and the schema
config variable must be set (to the same value). If the schema
config is set, but not the URL default schema, Nessie will try to re-create tables on the second run. I'm pretty sure I saw that in practice.
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.
Nessie will probably work fine with PosgreSQL if the schema is set in neither the JDBC URL, nor in the config.
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.
Hmm not sure, I just tried deployed a toy Nessie with this URL: jdbc:postgresql://nessie-postgres-postgresql:5432/nessiedb?currentSchema=nessie
and no schema
, and everything works as expected.
Then I tried jdbc:postgresql://nessie-postgres-postgresql:5432/
with catalog=nessiedb
and schema=nessie2
, it works too.
As for the second run issue, I'm surprised given how the code proceeds:
Lines 178 to 183 in e9d7beb
if (catalog == null || catalog.isEmpty()) { | |
catalog = conn.getCatalog(); | |
} | |
if (schema == null || schema.isEmpty()) { | |
schema = conn.getSchema(); | |
} |
If you saw this happening it would be good to have a reproducer.
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.
Hmm. Actually I just saw something similar. I think the code above might be a bit off. I'm investigating.
site/docs/guides/kubernetes.md
Outdated
secret: | ||
name: postgres-creds | ||
username: postgres_username | ||
password: postgres_password | ||
``` | ||
|
||
!!! note | ||
The required tables will be created automatically by Nessie if they don't exist, in the |
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.
(side note) do we have any control over this in our helm charts? I imagine two pods running DDL at the same time is not ideal 🤔
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 thought about that too, there is no formal control over that, and JdbcBackendBuilder.setupSchema()
could race and throw if called concurrently.
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.
"implicit schema creation" is a convenience thing. Sure - it may cause trouble for the first time.
I think in those cases it's not great that the first startup fails for the "additional pods".
It happens once - and k8s will likely restart the failed pod(s).
IMHO it's okay to just document that this can happen.
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.
Will Nessie Server stop on DDL failure? If yes, that should be good enough for k8s (I did not test :) )
On hold until we sort out #8723 . |
96fc24e
to
4e61b18
Compare
No description provided.