You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@poi.apache.org by Andreas Beeker <ki...@apache.org> on 2015/09/02 09:38:31 UTC
Time Zone Setting
Hi,
in the code we have some places where we reset the default time zone [1], like the following:
TimeZone cet = TimeZone.getTimeZone("Europe/Copenhagen");
TimeZone.setDefault(cet);
Although this is just in a junit test, I don't think it's a good idea to set the jdk default.
I've added a new ThreadLocal to DateUtil and although I'm not convinced, that this
is a definite solution, at least it's pragmatical:
@BeforeClass
public static void setTimeZone() {
userTimeZone = DateUtil.getUserTimeZone();
DateUtil.setUserTimeZone(TimeZone.getTimeZone("CET"));
}
Another solution would be, to guess the TimeZone by parsing date cells in the workbook
and compare them to the stored string values ... of course this only applies to X/HSLF.
What do you think about it?
Andi.
[1] org.apache.poi.hssf.usermodel.TestHSSFDateUtil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: Time Zone Setting
Posted by Dominik Stadler <do...@gmx.at>.
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 <ki...@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
Re: Time Zone Setting
Posted by Andreas Beeker <ki...@apache.org>.
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
RE: Time Zone Setting
Posted by Uwe Schindler <uw...@thetaphi.de>.
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.
>
> On Wed, Sep 2, 2015 at 9:38 AM, Andreas Beeker <ki...@apache.org>
> wrote:
> > Hi,
> >
> > in the code we have some places where we reset the default time zone
> [1], like the following:
> >
> > TimeZone cet = TimeZone.getTimeZone("Europe/Copenhagen");
> > TimeZone.setDefault(cet);
> >
> > Although this is just in a junit test, I don't think it's a good idea to set the jdk
> default.
> > I've added a new ThreadLocal to DateUtil and although I'm not
> > convinced, that this is a definite solution, at least it's pragmatical:
> >
> > @BeforeClass
> > public static void setTimeZone() {
> > userTimeZone = DateUtil.getUserTimeZone();
> > DateUtil.setUserTimeZone(TimeZone.getTimeZone("CET"));
> > }
> >
> > Another solution would be, to guess the TimeZone by parsing date cells
> > in the workbook and compare them to the stored string values ... of course
> this only applies to X/HSLF.
> >
> > What do you think about it?
> >
> > Andi.
> >
> >
> >
> > [1] org.apache.poi.hssf.usermodel.TestHSSFDateUtil
> >
> >
> > ---------------------------------------------------------------------
> > 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@poi.apache.org
For additional commands, e-mail: dev-help@poi.apache.org
Re: Time Zone Setting
Posted by Dominik Stadler <do...@gmx.at>.
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.
On Wed, Sep 2, 2015 at 9:38 AM, Andreas Beeker <ki...@apache.org> wrote:
> Hi,
>
> in the code we have some places where we reset the default time zone [1], like the following:
>
> TimeZone cet = TimeZone.getTimeZone("Europe/Copenhagen");
> TimeZone.setDefault(cet);
>
> Although this is just in a junit test, I don't think it's a good idea to set the jdk default.
> I've added a new ThreadLocal to DateUtil and although I'm not convinced, that this
> is a definite solution, at least it's pragmatical:
>
> @BeforeClass
> public static void setTimeZone() {
> userTimeZone = DateUtil.getUserTimeZone();
> DateUtil.setUserTimeZone(TimeZone.getTimeZone("CET"));
> }
>
> Another solution would be, to guess the TimeZone by parsing date cells in the workbook
> and compare them to the stored string values ... of course this only applies to X/HSLF.
>
> What do you think about it?
>
> Andi.
>
>
>
> [1] org.apache.poi.hssf.usermodel.TestHSSFDateUtil
>
>
> ---------------------------------------------------------------------
> 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