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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 40.86%. Comparing base (ad2646f) to head (f6bb6c4).
⚠️ Report is 2196 commits behind head on dev.

Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #10284      +/-   ##
============================================
- Coverage     40.86%   40.86%   -0.01%     
+ Complexity     4845     4844       -1     
============================================
  Files           885      885              
  Lines         35974    35976       +2     
  Branches       3993     3993              
============================================
- Hits          14702    14700       -2     
- Misses        19818    19820       +2     
- Partials       1454     1456       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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
caishunfeng reviewed Jun 9, 2022
View reviewed changes
Comment thread dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/JSONUtils.java Outdated
Comment thread dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/JSONUtils.java Outdated
@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