-
Notifications
You must be signed in to change notification settings - Fork 292
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
[AMORO-3340]: Database based http session store #3341
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # amoro-ams/src/main/java/org/apache/amoro/server/AmoroServiceContainer.java
…-session-store-db
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3341 +/- ##
============================================
+ Coverage 21.59% 27.48% +5.88%
- Complexity 2309 3509 +1200
============================================
Files 426 593 +167
Lines 39719 48295 +8576
Branches 5624 6231 +607
============================================
+ Hits 8577 13272 +4695
- Misses 30414 34087 +3673
- Partials 728 936 +208
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if (parameter == null) { | ||
if (SqlSessionFactoryProvider.getDbType().equals(AmoroManagementConf.DB_TYPE_POSTGRES)) { | ||
if (dbName.startsWith(AmoroManagementConf.DB_TYPE_POSTGRES)) { |
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 check source code and i find it's PostgreSQL
in org.postgresql.jdbc.PgDatabaseMetaData
that is not equails to postgres
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 dbName has been converted to lower 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.
the lower case postgresql
is also not equails to postgres
cache.setSessionDataStore(dataStore); | ||
// set session timeout | ||
Duration sessionTimeout = configurations.get(AmoroManagementConf.HTTP_SERVER_SESSION_TIMEOUT); | ||
handler.setMaxInactiveInterval((int) sessionTimeout.getSeconds()); |
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.
New to Javalin, Is that means we need to re-login after max_inactive_interval
?
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
Why are the changes needed?
Close #3340.
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation