Skip to content

Conversation

@raajeive
Copy link

@raajeive raajeive commented Apr 11, 2025

Added commandTimeoutMillis in executeRemoteCommand

Implemets #725

Copy link
Member

@tomaswolf tomaswolf left a comment

Choose a reason for hiding this comment

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

This needs a test, and must be done in a way that is backwards-compatible.

@raajeive raajeive force-pushed the master branch 4 times, most recently from 9dc97b6 to 7570eb5 Compare April 14, 2025 20:08
@raajeive raajeive changed the title Added commandTimeoutMillis in executeRemoteCommand [GH-725] Added commandTimeoutMillis in executeRemoteCommand Apr 15, 2025
@raajeive raajeive force-pushed the master branch 2 times, most recently from f564362 to 659d2ee Compare April 16, 2025 11:22
@raajeive raajeive requested a review from tomaswolf April 16, 2025 11:25
@raajeive
Copy link
Author

Hi @tomaswolf i have addressed all the comments provided, Can you please check this now?

@garydgregory
Copy link
Member

Hi all,

I think we should stop using ints and longs to express durations ASAP. Java has Duration class, let's use it. If a duration ends up as int seconds or long milliseconds or whatever scale a low level API in Java needs, that's fine, I just feel there is still room for confusion using primitives. A Duration also offers the best precision on Java 9 and up (nanosecond).

@raajeive
Copy link
Author

Hi @garydgregory

I've refactored the relevant code to use java.time.Duration instead of int/long for expressing timeouts. This should help improve clarity and avoid unit-related confusion going forward.

Would appreciate it if you could take a look and let me know if anything needs adjusting.

Thanks!

Added commandTimeoutMillis in executeRemoteCommand
@raajeive
Copy link
Author

raajeive commented Apr 23, 2025

Hi @tomaswolf @garydgregory,

Addressed all the comments, can you please check this and provide the approvals?

Thanks.

@tomaswolf tomaswolf merged commit 1cccb85 into apache:master Apr 28, 2025
7 checks passed
@tomaswolf
Copy link
Member

Thank you!

@raajeive
Copy link
Author

@tomaswolf @garydgregory @osi @knalli

When can we expect this to get released?

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

Successfully merging this pull request may close these issues.

3 participants