-
Notifications
You must be signed in to change notification settings - Fork 188
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
[#1865]: create processor to parse date time strings #2207
Conversation
Closes apache#1865 This processor parses a DateTime string. It converts the string into a ZonedDateTime variable, and places the variable in the event variable. Tests are made to ensure that the code works properly. I am still somewhat new, so any feedback is welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for providing this processor element @dshunter107 🙏🏼
This is really a decent start.
I've left you some remarks inline.
In addition, there is one thing missing for your pipeline element.
To get
.withLocales(Locales.EN)
.withAssets(Assets.DOCUMENTATION, Assets.ICON)
to work, we need to pass some resource files.
You can have a look in our docs or at this adapter for some references.
I'm happy to help wherever you need assistance 🙂
...ache/streampipes/processors/transformation/jvm/processor/datetime/TestDateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...ache/streampipes/processors/transformation/jvm/processor/datetime/TestDateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...ache/streampipes/processors/transformation/jvm/processor/datetime/TestDateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...ache/streampipes/processors/transformation/jvm/processor/datetime/TestDateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...ache/streampipes/processors/transformation/jvm/processor/datetime/TestDateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...g/apache/streampipes/processors/transformation/jvm/processor/datetime/DateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...g/apache/streampipes/processors/transformation/jvm/processor/datetime/DateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...g/apache/streampipes/processors/transformation/jvm/processor/datetime/DateTimeProcessor.java
Outdated
Show resolved
Hide resolved
...g/apache/streampipes/processors/transformation/jvm/processor/datetime/DateTimeProcessor.java
Outdated
Show resolved
Hide resolved
Incorporates feedback such as the following: * Way to initialize variables with constructors * simplifies methods with more efficient methods * creates the resource file for the processor * standardizes the names of inputs (i.e field) * creates the documentation for the processor
Okay. That clears up a few things for me. I have added the resource files. Thanks for all the feedback. I definitely feel like I am getting better with each pull request for each project. Everyone has been giving great feedback and helping me learn. Thanks!! |
Great to hear, hope you enjoy it 🙂 |
Yes, that's perfectly done now Yes there should already be the an element to create a dropdown, I'll have a look |
Thank you very much for your detailed explanation and deliberate thoughts 🙏🏼
With the context you provided, I'm totally fine with this solution 🙂 |
With respect to the dropdown menu, you can pass an additional boolean parameter to requiredSingleValueSelection(Labels.withId(SELECTED_INPUT_TIMEZONE),
Options.from(getTimeZoneOptions()),
true) Can you please verify this? Please don't ask why this parameter is called |
Also change inputTimeZone description in resource files
I am unable to verify. I am unable to use the docker commands with CLI because none of the Pipeline elements show up. When I start the program with docker-compose up -d in the compose folder, I am able to see all of the pipeline elements except the DateTimeFromStringDataProcessor. I probably did something incorrectly. I found the tutorial for DataProcessors at the following location: |
In case you want to continue here: one possible workaround here would be to pass the timestamp as long value including milliseconds instead of the local date time object diretly |
Take your time, No worries. Sure, whatever works. I included the timestamp as a long with milliseconds in the event as well as the time zone that the user specified. (1) Did you mean to say the "Zoned Date Time" object?
I ask because I am not sure if the timestampProperty was designed for ZonedDateTime. |
It is not ideal, but I'm afraid there is no real alternative in StreamPipes so far.
The output strategy is fine. Since you add the timezone as well, we should include this in the output strategy as well and adapt the documentation accordingly. The only aspect I'm unsure about is whether having multiple timestamp properties is a problem. @tenthe @dominikriemer What do you think? Will this lead anywhere to problems? |
One possible workaround is to use the time zone to reconstitute the datetime after the event has been collected.
Okay. I added the output strategy. Let me know if there is anything you would like me to do. |
Great, thank you! I'll give it a test in the upcoming days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some final improvements, hope you are okay with them 😊
Please apologize the long review process for your PR
Thanks for your contribution @dshunter107! |
You're welcome! Glad it works. |
Closes #1865
This processor parses a DateTime string. It converts the string into a ZonedDateTime variable, and places the variable in the event variable. Tests are made to ensure that the code works properly.
I am still somewhat new, so any feedback is welcome.
Purpose
Remarks
PR introduces (a) breaking change(s): no
PR introduces (a) deprecation(s): no