Uh oh!
There was an error while loading. Please reload this page.
CLOUDSTACK-10012: Jetty 9.4 - #2329
Conversation
marcaurele
commented
Nov 16, 2017
| http.port=8080 | ||
| # Max inactivity time in minutes for the session | ||
| session.timeout=10 |
There was a problem hiding this comment.
@marcaurele Can we bump the default to 30 minutes? I suppose 10 minutes might be too low, or maybe we can hit the sweet spot at 20mins?
There was a problem hiding this comment.
I agree that 10 minutes is very short. I would prefer 30
yadvr
commented
Nov 16, 2017
@blueorangutan package |
blueorangutan
commented
Nov 16, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 16, 2017
Packaging result: ✖centos6 ✔centos7 ✖debian. JID-1263 |
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
yadvr
commented
Nov 16, 2017
@marcaurele I've fixed the cloudian unit test failures and the session timeout to 30 (mins). |
yadvr
commented
Nov 16, 2017
@blueorangutan package |
blueorangutan
commented
Nov 16, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
yadvr
commented
Nov 16, 2017
@marcaurele how do you start the jetty server with mvn, when I run |
blueorangutan
commented
Nov 16, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1264 |
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
yadvr
commented
Nov 16, 2017
@marcaurele I found a workaround to use the old 9.2.x jetty-maven-plugin as the newer one did not work for me (and Travis). Please review the changes. |
yadvr
commented
Nov 16, 2017
@blueorangutan package |
blueorangutan
commented
Nov 16, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 16, 2017
Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1265 |
yadvr
commented
Nov 16, 2017
@blueorangutan package |
blueorangutan
commented
Nov 16, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 16, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1268 |
yadvr
commented
Nov 16, 2017
@blueorangutan test |
blueorangutan
commented
Nov 16, 2017
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
blueorangutan
commented
Nov 17, 2017
Trillian test result (tid-1658)
|
yadvr
commented
Nov 17, 2017
@blueorangutan package |
blueorangutan
commented
Nov 17, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 17, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1270 |
yadvr
commented
Nov 17, 2017
@blueorangutan test |
blueorangutan
commented
Nov 17, 2017
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>
marcaurele
commented
Nov 17, 2017
I moved the plugin version to root pom |
marcaurele
commented
Nov 17, 2017
LGTM |
| setKeystorePassword(properties.getProperty(KEYSTORE_PASSWORD)); | ||
| setWebAppLocation(properties.getProperty(WEBAPP_DIR)); | ||
| setAccessLogFile(properties.getProperty(ACCESS_LOG, "access.log")); | ||
| setSessionTimeout(Integer.valueOf(properties.getProperty(SESSION_TIMEOUT, "10"))); |
There was a problem hiding this comment.
@marcaurele you have a default value in the property file, which is 30, and here it is 10 (at line 85). Moreover, what about using the value of variable sessionTimeout as the default values? The variable already starts with the default value anyways.
There was a problem hiding this comment.
Thanks, I've synced default values to 30 now.
rafaelweingartner
left a comment
There was a problem hiding this comment.
LGTM from me here.
I only have a remark regarding the number of commits for such a small change. I understand that you have more than a person working here, but I believe the number of commits could be reduced.
yadvr
commented
Nov 17, 2017
@rafaelweingartner I'll squash merge them (I dislike splitting PRs in 10s+ of commits too) |
rafaelweingartner
commented
Nov 17, 2017
@rhtyd thanks! |
blueorangutan
commented
Nov 17, 2017
Trillian test result (tid-1659)
|
yadvr
commented
Nov 17, 2017
Tests LGTM, merged. Thanks everyone. |
andrijapanicsb
commented
Oct 30, 2019
Resurrecting an old PR - did anyone actually tested the session duration being set correctly (to something else than 30min)? It's not working in 4.11+ (whatever other value you set, the session is always set to 30min) @marcaurele@wido@rafaelweingartner /CC @rhtyd |
marcaurele
commented
Oct 31, 2019
@andrijapanicsb I'll have a look and come back here with the fix |
Upgrading to jetty 9.4 along with:
/to the context path