As I mentioned in the pull request (and especially since I advocated that constructor :-D), I like the constructor method. However, I don't feel there's enough compelling change to (a) remove existing constructors and (b) change all of the default configuration for a 3.5 release.
That just seems like making a change for the sake of making a change, not for the benefit of existing users. On Tue, Mar 13, 2012 at 9:49 PM, Andrew Petro <ape...@unicon.net> wrote: > Getting beyond milliseconds for timeouts feels like it will be more usable > for administrators configuring CAS servers. Which is to say, I agree with > the improvement intention articulated in CAS-1104. [1] Will anyone miss > being able to define timeouts in partial seconds, or is the > milliseconds-centric approach just adding three zeros to the end of each > configuration value? I expect the latter. > > There are some spots where minutes or even hours would be desirable as > configuration units. > > Yet, XML durations feels a complicated pill to swallow. They'd end up > looking something like > > PT5M > > for five minutes, a reasonable time to live for a service ticket, but > accidentally omitting the T > > P5M > > would mean five months, which is decidedly too long for a service ticket > to remain valid. > > > > This [2] looks beautifully literate: > > <bean id="grantingTicketExpirationPolicy" > class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy" > p:maxTimeToLiveInSeconds="28800" > p:timeToKillInSeconds="7200"/> > > Units are obviously seconds (says so right in the name). > > And yet, those values look like they really want to be hours, in that all > the numbers of seconds here are multiples of 3600. > > > I thought what Dima did here [2] was pretty interesting: > > <util:constant id="SECONDS" > static-field="java.util.concurrent.TimeUnit.SECONDS"/> > <bean id="serviceTicketExpirationPolicy" > class="org.jasig.cas.ticket.support.MultiTimeUseOrTimeoutExpirationPolicy" > c:numberOfUses="1" c:timeToKill="10" c:timeUnit-ref="SECONDS"/> > > That also reasonably literately says 10 seconds, without having to prepend > anything with "P" and get any "T"s into the right spot. > > > So maybe the former should be > > <util:constant id="SECONDS" > static-field="java.util.concurrent.TimeUnit.SECONDS"/> > <util:constant id="MINUTES" > static-field="java.util.concurrent.TimeUnit.MINUTES"/> > <util:constant id="HOURS" > static-field="java.util.concurrent.TimeUnit.HOURS"/> > > <bean id="grantingTicketExpirationPolicy" > class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy" > p:maxTimeToLiveUnit-ref="HOURS" > p:maxTimeToLive="8" > p:timeToKillUnit-ref="HOURS" > p:timeToKill="2"/> > > > And then, going after preferring constructor arguments [3] rather than > properties for required set-once configuration, and making use of that > beautiful c: prefix [4] to keep it literate: > > > <bean id="grantingTicketExpirationPolicy" > class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy" > c:maxTimeToLiveUnit-ref="HOURS" > c:maxTimeToLive="8" > c:timeToKillUnit-ref="HOURS" > c:timeToKill="2"/> > > > > How's that? We get units, it's constructor-injected, and the XML > configuration remains literate. > > If something like that does meet all the needs, then it ought to be doable > to find all the places where custom CAS code is currently configured in > milliseconds and instead accept a TimeUnit and a long. I'd think that can > be nailed for 3.5, how many such configuration points can there possibly > be? I'm willing to hunt them down and nail them if this solution is going > to otherwise fly. > > And I do think it's an improvement over configuration in milliseconds. > > Andrew > > [1]: https://issues.jasig.org/browse/CAS-1104 > > [2]: > https://github.com/Jasig/cas/blob/cbe1fa57ecda5c20ef892378c4742d5d528aa305/cas-server-webapp/src/main/webapp/WEB-INF/spring-configuration/ticketExpirationPolicies.xml > > [3]: > https://github.com/Jasig/cas/commit/11709f8294beb4cb1ffd699ca5cce6c14ef4383f > > [4]: > http://static.springsource.org/spring/docs/3.1.0.M1/spring-framework-reference/html/beans.html#beans-c-namespace > > > > On Mar 13, 2012, at 10:00 AM, Marvin S. Addison wrote: > > > Some recent commits have added additional properties/constructors to a > > couple ExpirationPolicy components that will be leveraged in the default > > Spring contexts for 3.5 and presumably future releases that change the > > default time interval from ms to s: > > > > > https://github.com/Jasig/cas/commit/11709f8294beb4cb1ffd699ca5cce6c14ef4383f > > > https://github.com/Jasig/cas/commit/a08f601e71b0abf98f5cda666144bce09c957638 > > > > While this may provide consistency for the expiration policies that are > > configured by default, the other ExpirationPolicy components still use > > milliseconds. Moreover, we have used ms fairly consistently for time > > intervals throughout the CAS codebase. > > > > The intent to increase convenience and clarity of time intervals for > > deployers is laudable and reasonable, but the changes above don't go > > nearly far enough. If tight timelines are limiting the scope of changes > to a few components, then it would be better to wait until we have the time > and resources to develop a flexible time interval facility and apply it > consistently. XML durations, http://www.w3.org/TR/xmlschema-2/#duration, > are one of a number of reasonable standards we might consider. > > > > I strongly recommend that we do no further harm with capricious > > component interface changes and tackle the issue thoroughly and > > consistently after the 3.5 release. I propose a Jira issue to track > > that effort and link it with the two issues behind the changes above. > > > > M > > > > -- > > You are currently subscribed to cas-dev@lists.jasig.org as: > ape...@unicon.net > > To unsubscribe, change settings or access archives, see > http://www.ja-sig.org/wiki/display/JSG/cas-dev > > > -- > You are currently subscribed to cas-dev@lists.jasig.org as: > scott.battag...@gmail.com > To unsubscribe, change settings or access archives, see > http://www.ja-sig.org/wiki/display/JSG/cas-dev > > -- You are currently subscribed to cas-dev@lists.jasig.org as: arch...@mail-archive.com To unsubscribe, change settings or access archives, see http://www.ja-sig.org/wiki/display/JSG/cas-dev
