Yes, you're right. Both static and final. :) 2009/6/27 sebb <seb...@gmail.com>
> On 27/06/2009, Xie Xiaodong <xxd82...@gmail.com> wrote: > > But how could ThreadLocal be shared among all AccessLogValve instances on > > the same server? After all, each thread access ThreadLocal has its own. > > > > Another point, based on the java doc of ThreadLocal, maybe "private > > ThreadLocal<AccessDateStruct> currentDateStruct" should be declared as > > static? > > +1, that should ensure that it will be shared. > > Also it should be declared final. > > > > > 2009/6/27 Konstantin Kolinko <knst.koli...@gmail.com> > > > > > > > Mark Thomas wrote: > > > >Konstantin Kolinko wrote: > > > >> 2. struct.currentDateString is calculated in two different places > > > >> I propose to encapsulate that code into AccessDateStruct > > > >> > > > >> 3. Invocation of getDate() in invoke() (the only place where > getDate() > > > >> is called): > > > >> I think that getDate() also can be incapsulated into > AccessDateStruct. > > > >> Actually, I think of the following: > > > >> 1) In invoke() there is already a variable that stores current > time > > > >> millis: "t2" > > > >> I propose to replace that getDate() with > > > >> AccessDateStruct.setDate(t2) that will perform the same job. > > > >> 2) Change the signature of replace(..., Date, ..) method, and pass > a > > > >> reference to the AccessDateStruct instead of a Date. > > > >> In the future the replace(..) methods of this class can be changed > to > > > >> accept a StringBuffer, instead of returning a String, but that can > be > > > >> another story. > > > >> > > > >> If AccessDateStruct is passed into the replace() method, > > > >> timeTakenFormatter can be added to it as well. Maybe. > > > >> > > > >> 4. The log() method still creates a new Date() once in a second. > > > >> The Date instance can be stored in some member variable and reused. > It > > > >> is in a synchronized block, so it will be safe. > > > >> I do not like, that it is the same once-in-a-second synchronized > > > >> block, that we already avoided once by using those ThreadLocals. > > > >> Can be addressed separately, though. > > > > > > > > Thanks for this. I'll see about updating the patch. > > > > > > > > > > One more thought: > > > It would be better if the ThreadLocal is shared among all > > > AccessLogValve instances on the same server. Otherwise, the count of > > > threads will be multiplied by the count of AccessLogValve instances. > > > > > > That implies that the AccessDateStruct does not have any configurable > > > parameters or any valve instance specific data. > > > E.g., fileDateFormat is configurable, thus its formatter is not a > > > candidate to be put there (and it is not there now). > > > > > > >> I do not like, that it is the same once-in-a-second synchronized > > > >> block, that we already avoided > > > I was not right writing this. We avoided sync around the formatters on > > > every call, and that makes a difference. > > > We already have sync in the writer on every call, so this sync > > > once-in-a second won't be something noticeable. Though maybe it can be > > > done once in 5/20/60 secs, but who cares. > > > > > > Best regards, > > > Konstantin Kolinko > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > > > For additional commands, e-mail: dev-h...@tomcat.apache.org > > > > > > > > > > > > > > -- > > Sincerely yours and Best Regards, > > > > Xie Xiaodong > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > -- Sincerely yours and Best Regards, Xie Xiaodong
