-
Notifications
You must be signed in to change notification settings - Fork 9
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
CORE-17154 - boot config for state manager #1242
CORE-17154 - boot config for state manager #1242
Conversation
Jenkins build for PR 1242 build 8 Build Successful: |
523b70b
to
c4f9834
Compare
c4f9834
to
0afc44c
Compare
47e9f32
to
9bbcd02
Compare
public static final String JDBC_DRIVER_DIRECTORY = DB_PROPERTIES + ".jdbc.directory"; | ||
public static final String JDBC_PERSISTENCE_UNIT_NAME = DB_PROPERTIES + ".jdbc.persistenceUnitName"; | ||
public static final String JDBC_POOL_MAX_SIZE = DB_PROPERTIES + ".pool.maxSize"; | ||
public static final String JDBC_POOL_MIN_SIZE = DB_PROPERTIES + ".pool.minSize"; | ||
public static final String JDBC_POOL_IDLE_TIMEOUT_SECONDS = DB_PROPERTIES + ".pool.idleTimeoutSeconds"; | ||
public static final String JDBC_POOL_MAX_LIFETIME_SECONDS = DB_PROPERTIES + ".pool.maxLifetimeSeconds"; | ||
public static final String JDBC_POOL_KEEP_ALIVE_TIME_SECONDS = DB_PROPERTIES + ".pool.keepAliveTimeSeconds"; | ||
public static final String JDBC_POOL_KEEP_ALIVE_TIME_SECONDS = DB_PROPERTIES + ".pool.keepaliveTimeSeconds"; |
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 noticed we've introduced slightly inconsistent naming for this property, I just want to keep it consistent
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've noticed the inconsistency, and I changed the name on purpose actually... the correct value is keepAliveTimeSeconds
so we should keep that instead of the old (wrong) one in my opinion 🤷♂️
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 not sure we can actually change the others (crypto and rbac) at this stage, that's probably a breaking change? I'm assuming this values schema is considered part of our public API?
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.
We probably can't without breaking backward compatibility, true, but why keep the wrong name in newly introduced API instead of using the correct one?.
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.
It's unfortunately spread around the codebase quite a bit, I'd prefer we named it proper camelcase but my gut tells me to keep it consistent. I will change it back since we can't really change the others, at least stateManager will be correct.
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
Introduces boot config keys for the state manager. Follows on from 1240.
Runtime-os change: corda/corda-runtime-os#4624