You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/11/19 19:32:07 UTC
[GitHub] [nifi] adenes opened a new pull request #4677: NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is…
adenes opened a new pull request #4677:
URL: https://github.com/apache/nifi/pull/4677
… CET
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFI-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [x] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4677: NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4677:
URL: https://github.com/apache/nifi/pull/4677#issuecomment-731351810
Thanks for the background @adenes that is helpful to know. I agree with you that the underlying issue is related to using `java.sql.Date` for representing dates without times.
Looking at the unit test you referenced in TestConvertRecord and stepping through the code, it looks like the better place to address the problem is in `DataTypeUtils.toDate()` where `DateFormat.parse()` creates a `java.util.Date`. As you described, when walking through the unit test, the parse result returns a `java.util.Date` with a local time zone, which skews the resulting `java.sql.Date`. Replacing DateFormat with `LocalDate.parse()` and using `java.sql.Date.valueOf()` avoids the skewed return value. Making the change at that level would avoid the need to readjust values later on.
Making the change in `DataTypeUtils.toDate()` will probably have other consequences, but that seems like it is worth considering to get to the root of the problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #4677: NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is…
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #4677:
URL: https://github.com/apache/nifi/pull/4677#issuecomment-767457413
Closing in favor of implementation in #4781
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes commented on pull request #4677: NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is…
Posted by GitBox <gi...@apache.org>.
adenes commented on pull request #4677:
URL: https://github.com/apache/nifi/pull/4677#issuecomment-731271678
Thanks @exceptionfactory for the review. Actually your recommended change is the revert of the original commit that caused these test to fail (see: https://github.com/apache/nifi/commit/83948db989bf2f682dfa18c56293ec431f546e1c#diff-98d8c798133bf7b6d8a2b49a35139863b6ac85c6001787a62b3c6117abafb440L669)
I think we have a more generic issue, using `java.sql.Date` for representing dates is not the best solution. Being a subclass of `java.util.Data` means that under the hood it stores the time too, just doesn't expose it.
Storing the time leads to a "date skew" issue: when 2020-11-20 is represented as 2020-11-20 00:00 UTC but if later it's converted to a different timezone (e.g. EST, UTC-5) the same instant is presented as 2020-11-19 19:00 EST, thus the date part has changed.
This case is covered by a newly added test case: https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestConvertRecord.java#L378
My opinion is that in case of dates time zone doesn't make sense (i.e. 2020-11-19 is the same date in every time zone, even if in some it starts earlier), that's why I removed the `toLocalDate()` call in the previously mentioned commit to fix this issue.
Unfortunately this led to these two failing tests, but I think the tests are incorrect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4677: NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4677:
URL: https://github.com/apache/nifi/pull/4677#issuecomment-731252957
Looking at the issue in more detail, did you evaluate AvroTypeUtil.convertToAvroObject() and how it handles the LOGICAL_TYPE_DATE? The current implementation converts from java.sql.Date.getTime() to Instant, using Instant.ofEpochMilli() and passes in the milliseconds from java.sql.Date.getTime().
Since java.sql.Date does not include the timezone, the ChronoUnit.DAYS.between() calculation returns a different number of days depending on the JVM timezone.
Changing AvroTypeUtil.convertToAvroObject() line 669 to use java.sql.Date.toLocalDate().toEpochDay() results in both tests working without any changes, and appears to operate according to the original intent of the method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] adenes closed pull request #4677: NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is…
Posted by GitBox <gi...@apache.org>.
adenes closed pull request #4677:
URL: https://github.com/apache/nifi/pull/4677
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org