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

Add new flux command sleep #495 #559

Merged
merged 10 commits into from
Nov 28, 2024
Merged

Add new flux command sleep #495 #559

merged 10 commits into from
Nov 28, 2024

Conversation

TobiasNx
Copy link
Contributor

@TobiasNx TobiasNx commented Sep 24, 2024

Resolves #495

@TobiasNx TobiasNx requested review from fsteeg and removed request for fsteeg September 26, 2024 14:37
@TobiasNx
Copy link
Contributor Author

@fsteeg could you have a look, if this would be way worth to continue

@fsteeg
Copy link
Member

fsteeg commented Sep 27, 2024

Basic idea looks good. Some issues:

  • PR description should reference the issue using closing keywords
  • I'd copy an existing module, e.g. ObjectExceptionCatcher to get the types right (current build: does not compile cannot find symbol [...] class T)
  • Formatting is off (comments), would also be avoided by copying an existing module
  • I think the flux command should be just sleep
  • The flux command needs to be added in src/main/resources/flux-commands.properties
  • The Thread.sleep needs to be wrapped in a try-catch-block, handling InterruptedException, see e.g. in the OERSI SitemapReader

@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Sep 27, 2024
@TobiasNx
Copy link
Contributor Author

Basic idea looks good. Some issues:

* PR description should reference the issue using closing keywords

* I'd copy an existing module, e.g. ObjectExceptionCatcher to get the types right (current build: does not compile `cannot find symbol [...] class T`)

* Formatting is off (comments), would also be avoided by copying an existing module

* I think the flux command should be just `sleep`

* The flux command needs to be added in `src/main/resources/flux-commands.properties`

* The `Thread.sleep` needs to be wrapped in a try-catch-block, handling `InterruptedException`, see e.g. in the OERSI [SitemapReader](https://gitlab.com/oersi/oersi-etl/-/blob/master/src/main/java/oersi/SitemapReader.java?ref_type=heads#L65)

Thanks I adjusted as suggested.
It is missing java tests. I am not sure how to add them for this specific function.

@TobiasNx TobiasNx changed the title Add draft for object sleeper #495 Add new flux command sleep #495 Oct 17, 2024
@TobiasNx TobiasNx marked this pull request as ready for review October 17, 2024 10:20
@TobiasNx TobiasNx assigned fsteeg and unassigned TobiasNx Oct 17, 2024
@TobiasNx TobiasNx requested a review from fsteeg October 17, 2024 10:38
@blackwinter
Copy link
Member

Would it make sense to enable this command to also support the Fix function use case? Just add another setter for the time unit and extract the sleep "action" into a dedicated method. Then we could easily reuse this class to implement the corresponding Fix function (see Catmandu).

@fsteeg fsteeg assigned TobiasNx and unassigned fsteeg Oct 18, 2024
@TobiasNx
Copy link
Contributor Author

Would it make sense to enable this command to also support the Fix function use case? Just add another setter for the time unit and extract the sleep "action" into a dedicated method. Then we could easily reuse this class to implement the corresponding Fix function (see Catmandu).

I think this is a good idea, could you provide the code for this?

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx Oct 29, 2024
@blackwinter
Copy link
Member

I think this is a good idea, could you provide the code for this?

Implemented in f069a04 and 18b6ae0.

Note that Catmandu defaults to SECONDS, and doesn't support NANOSECONDS and DAYS.

@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Nov 6, 2024
@TobiasNx TobiasNx requested a review from fsteeg November 11, 2024 07:51
@TobiasNx
Copy link
Contributor Author

@fsteeg can you review the additions that @blackwinter introduced. Also can you tell me how I would write a unit test for this? Not sure since it does change the result in a transformation.

@blackwinter
Copy link
Member

Also can you tell me how I would write a unit test for this?

Initially, I thought we should just skip the test for this. But since you brought it up: You could verify that the process takes at least the amount of time that it was configured to sleep. It's not pretty, but it's something.

Not sure since it does change the result in a transformation.

You mean "it does not change the result", right?

@TobiasNx
Copy link
Contributor Author

Also can you tell me how I would write a unit test for this?

Initially, I thought we should just skip the test for this. But since you brought it up: You could verify that the process takes at least the amount of time that it was configured to sleep. It's not pretty, but it's something.

Okay, I will have a look at this.

Not sure since it does change the result in a transformation.

You mean "it does not change the result", right?

Correct, it does not change the result

@TobiasNx TobiasNx removed their assignment Nov 27, 2024
@TobiasNx TobiasNx requested a review from blackwinter November 27, 2024 16:57
@TobiasNx
Copy link
Contributor Author

Also can you tell me how I would write a unit test for this?

Initially, I thought we should just skip the test for this. But since you brought it up: You could verify that the process takes at least the amount of time that it was configured to sleep. It's not pretty, but it's something.

Not sure since it does change the result in a transformation.

You mean "it does not change the result", right?

@blackwinter is this the kind of test you had in mind?

@blackwinter
Copy link
Member

Yes, thanks. However, I've modified your test a little (74bc15d).

Stylistic changes:

  1. Switch from Instant/Duration to nanoTime().
  2. Avoid static import statements.
  3. Declare local variables as final.
  4. Provide failure message if assertion fails.
  5. Clean up formatting/indentation.

Functional changes:

  1. Verify that sleep time doesn't exceed expected time by too much (otherwise you could make all tests pass by always sleeping for a very long time).
  2. Add tests with explicit time unit.

@blackwinter blackwinter assigned TobiasNx and unassigned blackwinter Nov 28, 2024
@TobiasNx TobiasNx merged commit 4ab2474 into master Nov 28, 2024
1 check passed
@TobiasNx TobiasNx deleted the 495-objectSleep branch November 28, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add a flux command for waiting a specific time
3 participants