Skip to content

[Fix] Fix the JSONUtils tool class time zone problem #10282#10284

Merged
caishunfeng merged 5 commits intoapache:devfrom
106umao:fix#10282
Jun 17, 2022
Merged

[Fix] Fix the JSONUtils tool class time zone problem #10282#10284
caishunfeng merged 5 commits intoapache:devfrom
106umao:fix#10282

Conversation

@106umao
Copy link
Copy Markdown
Contributor

@106umao 106umao commented May 29, 2022 •
edited
Loading

Purpose of the pull request

Fixed #10282

Brief change log

Add static method setTimeZone(TimeZone timeZone) for class org.apache.dolphinscheduler.common.utils.JSONUtils
The purpose of the method is to support dynamic modification of the TimeZone setting of the ObjectMapper object

Verify this pull request

This pull request is already covered by existing tests, such as (please describe tests).

method org.apache.dolphinscheduler.common.utils.JSONUtilsTest#dateToString
method org.apache.dolphinscheduler.common.utils.JSONUtilsTest#stringToDate

This change added tests and can be verified as follows:

Run mvn clean test in terminal

@106umao 106umao mentioned this pull request May 29, 2022
Closed
3 tasks
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 29, 2022 •
edited
Loading

Codecov Report

Merging #10284 (f6bb6c4) into dev (ad2646f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##                dev   #10284   +/-   ##
=========================================
  Coverage     40.85%   40.86%           
- Complexity     4843     4844    +1     
=========================================
  Files           885      885           
  Lines         35974    35976    +2     
  Branches       3993     3993           
=========================================
+ Hits          14698    14700    +2     
  Misses        19820    19820           
  Partials       1456     1456
Impacted Files Coverage Δ
...pache/dolphinscheduler/common/utils/JSONUtils.java 69.79% <100.00%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad2646f...f6bb6c4. Read the comment docs.

@songjianet songjianet added bug Something isn't working backend labels May 30, 2022
@songjianet songjianet added this to the 3.0.0-beta-2 milestone May 30, 2022
@106umao
Copy link
Copy Markdown
Contributor Author

106umao commented May 30, 2022

Hi @SbloodyS @zhuangchong
Can anyone help me review？

@SbloodyS SbloodyS requested a review from caishunfeng May 30, 2022 14:57
@caishunfeng
Copy link
Copy Markdown
Contributor

caishunfeng commented Jun 6, 2022

The issue had been fixed #10345

@SbloodyS SbloodyS mentioned this pull request Jun 6, 2022
Closed
@zhongjiajie
Copy link
Copy Markdown
Member

zhongjiajie commented Jun 8, 2022

The issue had been fixed #10345

So we do not need this anymore, is it?

@106umao
Copy link
Copy Markdown
Contributor Author

106umao commented Jun 8, 2022

@zhongjiajie
@caishunfeng
Hi, it is still fail.
image

@SbloodyS
Copy link
Copy Markdown
Member

SbloodyS commented Jun 8, 2022 •
edited
Loading

@zhongjiajie @caishunfeng Hi, it is still fail. image

The reason of still failed is that the github action's host machine's timezone is UTC by default. But your local timezone is not.

@106umao
Copy link
Copy Markdown
Contributor Author

106umao commented Jun 8, 2022

@SbloodyS
This time I changed the time zone of the system

image

@caishunfeng
Copy link
Copy Markdown
Contributor

caishunfeng commented Jun 9, 2022

@zhongjiajie @caishunfeng Hi, it is still fail. image

The reason of still failed is that the github action's host machine's timezone is UTC by default. But your local timezone is not.

As @SbloodyS said, I think we need this pr. @106umao please solve the conflicts, thanks.

@SbloodyS SbloodyS added the test label Jun 17, 2022
@SbloodyS
Copy link
Copy Markdown
Member

SbloodyS commented Jun 17, 2022 •
edited
Loading

Hi @106umao , I have noticed that many CI failed for this reason. So I helped you solve the conflict and optimize it. Hoped you don't mind.

@SbloodyS
Update JSONUtilsTest.java
f6bb6c4 
add timezone import
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jun 17, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

caishunfeng
caishunfeng approved these changes Jun 17, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@caishunfeng caishunfeng merged commit 062146e into apache:dev Jun 17, 2022
@106umao
Copy link
Copy Markdown
Contributor Author

106umao commented Jun 17, 2022

Hi @106umao , I have noticed that many CI failed for this reason. So I helped you solve the conflict and optimize it. Hoped you don't mind.

@SbloodyS @caishunfeng thx.

@zhongjiajie zhongjiajie mentioned this pull request Jun 18, 2022
Merged
hstdream pushed a commit to hstdream/dolphinscheduler that referenced this pull request Jun 23, 2022
@106umao @SbloodyS @hstdream
[Fix] Fix the JSONUtils tool class time zone problem apache#10282 (ap…
cc64ce4 
…ache#10284)

* [Fix] Fix the JSONUtils tool class time zone problem apache#10282

* [Fix] Fix the JSONUtils tool class time zone problem apache#10282

* Update JSONUtils.java

remove unnessnary log

* Update JSONUtilsTest.java

add timezone import

Co-authored-by: xiangzihao <460888207@qq.com>
caishunfeng pushed a commit to caishunfeng/dolphinscheduler that referenced this pull request Jul 19, 2022
@106umao @SbloodyS @caishunfeng
[Fix] Fix the JSONUtils tool class time zone problem apache#10282 (ap…
ae4c3ab 
…ache#10284)

* [Fix] Fix the JSONUtils tool class time zone problem apache#10282

* [Fix] Fix the JSONUtils tool class time zone problem apache#10282

* Update JSONUtils.java

remove unnessnary log

* Update JSONUtilsTest.java

add timezone import

Co-authored-by: xiangzihao <460888207@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caishunfeng caishunfeng caishunfeng approved these changes

Assignees

@106umao 106umao

Labels

backend bug Something isn't working test

Projects

None yet

Milestone

3.0.0-beta-2

Development

Successfully merging this pull request may close these issues.

[Bug] [dolphinscheduler-common] JSONUtils tool class time zone problem reporting error

6 participants

@106umao @codecov-commenter @caishunfeng @zhongjiajie @SbloodyS @songjianet