Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove joda time dependency from maestro-common #25

Open
jun-he opened this issue Jul 23, 2024 · 6 comments
Open

remove joda time dependency from maestro-common #25

jun-he opened this issue Jul 23, 2024 · 6 comments
Assignees

Comments

@jun-he
Copy link
Contributor

jun-he commented Jul 23, 2024

as it has been deprecated for a long time.

@pranaybattu
Copy link

Hi @jun-he ,

I'd like to take on this task of removing the Joda-Time dependency from the maestro-common module.

Could you please assign this issue to me?

Additionally, I would like to clarify a few points:

Are there any specific parts of the codebase that have critical dependencies on Joda-Time, which I should be particularly careful about?

pranaybattu pushed a commit to pranaybattu/maestro that referenced this issue Jul 25, 2024
- Removed Joda-Time dependency from the Defaults class and replaced it with java.time.ZoneId.
- Updated TriggerHelper class to use java.time.ZoneId instead of Joda-Time's DateTimeZone.
- Simplified TimeZoneValidator to use a static final set of available time zone IDs, ensuring efficient validation.
@pranaybattu
Copy link

@jun-he

Please review the changes related to the removal of the Joda-Time dependency from the maestro-common module.

Thank you!

@pranaybattu
Copy link

Found a similar issue in netflix-sel where there is an interdependency due to passing DateTimeZone in SEL.

Here is the relevant code snippet for the request body of the "Create a sample workflow" API:

{
    "params": {
        "foo": {
            "expression": "new DateTime(1569018000000).withZone(DateTimeZone.forID('UTF')).monthOfYear().getAsText();",
            "type": "STRING"
        }
    }
}

@pranaybattu
Copy link

APIs are working fine, we can create an issue for the removal of joda time from netflix-sel

@jun-he
Copy link
Contributor Author

jun-he commented Jul 26, 2024

@pranaybattu thanks for the contribution! Yes, SEL also uses it and it needs some big changes to move away from it. Basically, we have to implement those SEL method using Java time libraries than Joda.
So we can focus on remove it from other places in this issue. I will take a look the PR soon. Thanks.

@jun-he
Copy link
Contributor Author

jun-he commented Jul 26, 2024

I created a new issue for that: #61

pranaybattu pushed a commit to pranaybattu/maestro that referenced this issue Aug 4, 2024
- Removed Joda-Time dependency from the Defaults class and replaced it with java.time.ZoneId.
- Updated TriggerHelper class to use java.time.ZoneId instead of Joda-Time's DateTimeZone.
- Simplified TimeZoneValidator to use a static final set of available time zone IDs, ensuring efficient validation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants