poi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dominik Stadler <dominik.stad...@gmx.at>
Subject Re: Time Zone Setting
Date Thu, 03 Sep 2015 20:15:41 GMT
Hi,

Using the ".getDefault()" values sounds good, as this way we run less
risk of breaking existing stuff.

But overall it would be good if we can get back to a "green" build
again somehow first, I tried to see what changes are necessary right
now to make tests work, but the tests all are quite complicated and
not easy to fix for me. Should we take a step back and revert some of
those "forbidden-api-check" changes and do them in a more step-by-step
manner?

Dominik.

On Thu, Sep 3, 2015 at 5:10 PM, Andreas Beeker <kiwiwings@apache.org> wrote:
> Hi,
>
> I have a lot of test errors and already fixed quite a lot, but haven't committed them
yet.
>
> Short version:
> how about ... instead of using Locale.ROOT and Timezone UTC,
> the initial value of the ThreadLocal return Locale.getDefault() and Timezone.getDefault()
> and can be set if needed ... so the handling is similar to older versions?
>
> Long version:
> When I've started to fix the forbidden api errors, I've only touched places,
> where I was quite sure that Locale.ROOT won't do any harm.
> But of course now we also have to face the other issues...
>
> The reason for having a thread local is ...:
> - that having the timezone/locale as an additional parameter would lead to a lot of methods
>   to be marked deprecated (at least I would do it...)
> - I've read about use-cases where workbooks from different time zones need to
>   be processed, so it's not only the current environment time zone
>
> - "A ThreadLocal can lead to hard-to-find errors ...":
>   I think the hardest part is, making the new handling public.
>   Another problem I see with thread locals is, the reuse of servlet threads.
>   So the usage pattern would need to restore the original setting in a finally block.
>
> - "... in fact almost anybody would now see UTC-formatted dates instead
>    of local timezone, or?"
>   I guess so too, and the tests fail when my CET-timezone is ignored
>   (the DE-locale is only affecting very few cases), i.e. I have to set it explicitly
>
> - "The java default is to either use the system default ..."
>   As noted above, I would like to provide a multi timezone/locale approach, but
>   still keep a lot of the poi api as-is.
>
> I guess the discussion will continue for a bit, so I create a patch file instead of committing
> the changes and check your feedback then.
> If you commit changes in the meantime, I'll update my version accordingly, so don't be
> blocked ;)
>
> Andi
>
>
> On 03.09.2015 16:25, Uwe Schindler wrote:
>> Hi,
>>
>> This may be only slightly related: to work around the forbidden-apis issue, you can
still pass Timezone.getDefault() to the Java APIs - this is not forbidden. Forbidden APIs
just wants to make sure you know what you are doing. By writing Timezone.getDefault() you
explicitely say what you are doing. And maybe add a comment why you do this. The same applies
for Locales or Charsets. E.g., At some places in Lucene we explicitely use the charset of
the console, so we use Charset.getDefault() at all those places.
>>
>> Uwe
>>
>> -----
>> Uwe Schindler
>> H.-H.-Meier-Allee 63, D-28213 Bremen
>> http://www.thetaphi.de
>> eMail: uwe@thetaphi.de
>>
>>
>>> -----Original Message-----
>>> From: Dominik Stadler [mailto:dominik.stadler@gmx.at]
>>> Sent: Thursday, September 03, 2015 4:05 PM
>>> To: POI Developers List
>>> Subject: Re: Time Zone Setting
>>>
>>> Hi,
>>>
>>> Sorry, but I fear that through this we change the default behavior of POI:
>>> * A ThreadLocal can lead to hard-to-find errors when people use POI in a
>>> multi-threading environment. While it is better in some cases than having a
>>> global static, it now requires the developer to ensure that the timezone is set
>>> for each thread that is started without a way to set it globally.
>>> * I think previously POI used the java default settings which look at the
>>> operating system, this is now broken as we have "UTC" as default, which
>>> potentially leads to many people seeing different results compared to
>>> before, in fact almost anybody would now see UTC-formatted dates instead
>>> of local timezone, or?
>>> * The java default is to either use the system default timezone or if it should
>>> be different then use -Duser.timezone when invoking the JRE, see e.g.
>>> http://stackoverflow.com/questions/2493749/how-to-set-a-jvm-timezone-
>>> properly
>>>
>>> So my approach would be to:
>>> * Not use the DateUtil.setTimezone() approach as it has quite some potential
>>> for regressions but also not use TimeZone.setDefault() in unit tests at all
>>> * Ensure that we set -Duser.timezone to a fixed value when running unit
>>> tests in the various CI systems. We already do this for user.language and
>>> user.country, so setting the timezone as well here seems a natural choice
>>>
>>> This way we do not change default behavior for users and still make the CI
>>> runs a bit more reproducible time-wise...
>>>
>>> Dominik.
>>>
>>>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
> For additional commands, e-mail: dev-help@poi.apache.org
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org


Mime
View raw message