[2743] Migrate internal use of CircuitBreaker double to duration #2748

mjmoore · 2022-06-25T07:01:25Z

Changes internal usage of Double to Duration.
No changes made to public factory methods (CircuitBreaker::of). Questionable to accept a Duration, convert it to Double and then covert it back to a Duration again to pass to the internal constructor. Would appreciate some guidance here on how to manage the @ExperimentalTime annotation here.

nomisRev · 2022-06-27T12:00:21Z

@mjmoore thanks for this awesome contribution, and congrats on your first contribution 👏 👏 👏 🥳

The CI is currently failing because of some changes in the public API, can you run ./gradlew apiDump and push the changes? There are more details on this here if you're interested.

I think we should also deprecate the Double constructor, so users will be able to automatically refactor.
Full example:

@Deprecated(
   "Prefer the kotlin.time.Duration constructor instead",
    ReplaceWith(
      "of(maxFailures, resetTimeoutNanos.nanoseconds, exponentialBackoffFactor, maxResetTimeout, onRejected, onClosed, onHalfOpen, onOpen)",
      "import kotlin.time.Duration.Companion.nanoseconds"
    )
)

Would appreciate some guidance here on how to manage the @ExperimentalTime annotation here.

The kotlin.time.Duration was stabilised in 1.6.0 so we should be able to simply remove the annotations. 1.6.0 release docs, or is it requiring @ExperimentalTime for something else in the kotlin.time.* package?

mjmoore · 2022-06-28T06:44:37Z

@nomisRev Thanks for the guidance! Should look a little better now 🙂

nomisRev

Looks great to me @mjmoore. Nice work 👏 👏 👏

A couple suggestions for improving the error messages for the preconditions of the constructors. Also it was missing && it != Duration.ZERO check. If you agree with my suggestions, I think it's ready to be merged 🙌

So this is probably also missing tests. Would be great if you don't mind adding one, but not a blocker for this PR ;)

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CircuitBreaker.kt

mjmoore · 2022-07-05T07:45:25Z

@nomisRev Thanks for the comments!

Updated validation checks.
Added some basic tests for constructor validation.
Removed duplicated validation. Validation is now performed only in the primary CircuitBreaker::of constructor.
Reworked the terse validation logic to something a bit more readable.

i-walker · 2022-07-07T06:09:08Z

arrow-libs/fx/arrow-fx-coroutines/api/arrow-fx-coroutines.api

 	public fun equals (Ljava/lang/Object;)Z
-	public final fun getResetTimeoutNanos ()D
+	public final fun getResetTimeout-UwyO8pc ()J


~~can we redump the api file, just to check if that hash goes away?~~

It seems that every time we use Duration it produces this binary output and changes the descriptor Name for instance here https://github.com/nomisRev/ArrowDokkaTest/blob/507f6addb1e379f84aa4257d14b002fc4fcfac7f/arrow-libs/fx/arrow-fx-coroutines/api/arrow-fx-coroutines.api#L57

i-walker

Thanks @mjmoore 🙌🏾

nomisRev

Thank you for the contribution @mjmoore! 🙏 Was my pleasure to work together ;)

nomisRev · 2022-07-07T08:50:21Z

...-libs/fx/arrow-fx-coroutines/src/commonTest/kotlin/arrow/fx/coroutines/CircuitBreakerTest.kt

+      ConstructorValues(maxResetTimeout = Duration.ZERO),
+      ConstructorValues(maxResetTimeout = (-1).seconds),
+    ).forEach { value ->
+      "should require valid constructor values" {


binary breaking change in HalfOpen and Open public resetTimeOut variable

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CircuitBreaker.kt

nomisRev · 2022-07-16T10:11:16Z

@mjmoore Sorry for the long wait. I had a busy week, but I was finally able to dedicate some time to this again.

We should really maintain binary compatibility so we don't break libraries depending on older 1.x.x versions of Arrow,
but it was trickier than I thought with these @JvmInline value class.
I was able to maintain binary compatibility by shuffling around some constructors, and properties.

Feel free to copy the changes, or cherry-pick this commit whatever makes most sense for you 👍
1c0bfe6

nomisRev · 2022-07-16T10:11:38Z

PS: @i-walker this fixes the binary compat issues. Thanks for watching out for this 🙏

mjmoore · 2022-09-06T08:28:35Z

Hey @nomisRev, @i-walker, finally coming back to this. Cherry picked 1c0bfe6, should be good to go now hopefully.

nomisRev · 2022-09-06T09:58:15Z

Hey @mjmoore, awesome! 🙌 I'll review it asap 😊

arrow-libs/fx/arrow-fx-coroutines/api/arrow-fx-coroutines.api

i-walker

Thanks @mjmoore 🙌🏾

mjmoore · 2022-09-22T09:29:03Z

Hey @nomisRev, looks like the previous build failed due to a timeout when configuring arrow-optics. I've rebased in case there were any issues with main. Would be great if you could kick off a new build.

nomisRev · 2022-09-22T11:12:32Z

Hey @mjmoore,
Yes, that is related to a Gradle bug that will be fixed in the next version.

gradle/gradle#19446 (comment).

nomisRev

Thank you so much for @mjmoore for your first-contribution 🥳
Really nice work venturing in Arrow Fx straight away 🙌
Apologies that it took so long to get your contribution in, and thanks for your patience 🙏

* Add Resource.allocated() to decompose Resource into it's allocate and… (#2820) * [2743] Migrate internal use of CircuitBreaker double to duration (#2748) * Fix typo (#2824) * Make the server disconnect test more general (#2822) * Update NonEmptyList.fromList deprecation to suggest `toOption()` instead (#2832) * Improve Either.getOrHandle() docs (#2833) * Improve allocated, and fix knit examples Co-authored-by: Jeff Martin <jeff@custommonkey.org> Co-authored-by: Martin Moore <martinmoorej@gmail.com> Co-authored-by: valery1707 <valery1707@gmail.com> Co-authored-by: Lukasz Kalnik <70261110+lukasz-kalnik-gcx@users.noreply.github.com> Co-authored-by: stylianosgakis <stylianos.gakis98@gmail.com>

* The beginning of Arrow 2.0 * Resource Arrow 2.0 (#2786) * Flatten Resource ADT, maintain API * [Arrow 2.0] Effect without suspending shift (#2797) * Shift without suspend, inline all the rest * Add new error handlers signatures * Make ShiftCancellationException private * Rename Shift to Raise according to Slack Poll, and add some initial docs (#2827) * Remove all references to shift from new Arrow 2.0 code (#2834) * Remove all references to shift from new code * Update API files * Fixes merge conflict between main and arrow-2 (#2835) * Add Resource.allocated() to decompose Resource into it's allocate and… (#2820) * [2743] Migrate internal use of CircuitBreaker double to duration (#2748) * Fix typo (#2824) * Make the server disconnect test more general (#2822) * Update NonEmptyList.fromList deprecation to suggest `toOption()` instead (#2832) * Improve Either.getOrHandle() docs (#2833) * Improve allocated, and fix knit examples Co-authored-by: Jeff Martin <jeff@custommonkey.org> Co-authored-by: Martin Moore <martinmoorej@gmail.com> Co-authored-by: valery1707 <valery1707@gmail.com> Co-authored-by: Lukasz Kalnik <70261110+lukasz-kalnik-gcx@users.noreply.github.com> Co-authored-by: stylianosgakis <stylianos.gakis98@gmail.com> * Add Atomic module, and StateShift (#2817) * Two small deprecations * Add Atomic module, and StateShift. Implement ior through StateShift * Fix build * Fix atomic knit * Fix knit attempt #2 * Update API files * Remove references to shift * Change return type of raise to Nothing (#2839) * Change return type of raise to Nothing Implements #2805 * Update public api with ./gradlew apiDump * Suppress warnings for unreachable calls to fail in tests The test is verifying that no code will be executed after a call to raise, so it's actually testing the very behaviour that the compiler is warning us about. Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update API files * Increase timeout * Fix compiler bug with nested inline + while + return * Clean up ExitCase.fromError * Update API files@ * Feature/remove validated (#2795) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Remove CancellationExceptionNoTrace.kt after merge, and run apiDump * Add missing runnerJUnit5 for arrow-atomic JVM * Publish Arrow 2.0.0-SNAPSHOT (#2871) Co-authored-by: Javier Segovia Córdoba <7463564+JavierSegoviaCordoba@users.noreply.github.com> * Simplify optics to Traversal/Optional/Lens/Prism (#2873) * 'mapOrAccumulate' for Raise (#2872) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Fix problems with Atomic * Smaller timeouts * Remove Tuple10 to Tuple22, Const, Eval, computation blocks, and arrow-continuations (#2784) * Revert typo * Fix build * Fix ParMapJvmTest * Implement NonEmptyList using value class (#2911) * Fix merge w.r.t. Saga * apiDump * Test other return expression * change unalign signature (#2972) see discussion in #2960 (comment) #2960 (comment) --------- Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update after merge main * Fix :arrow-optics-ksp-plugin:compileKotlin * Fix Every instances * Move functions to arrow functions (#3014) * Bring back `Iso` (#3013) * Bring back Iso * API Dump --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update BOM (#3019) * Fix andThen duplication (#3020) * Fix Knit * Fix weird problem with value classes * Update API docs * Update publish workflow Following the instructions in #3090 (comment) * No closing repo on snapshot * knit * Fix optics tests * Fix after merge * Refactor ParMapTest from Kotest Plugin to Kotlin-test runtime #3191 (#3221) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor: Use Kotlin-test runtime for arrow-fx-stm tests (#3224) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update all Gradle files to mention kotlin.test * Refactor ParZip2Test from Kotest Plugin to Kotlin-test runtime #3192 (#3222) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor ParZip3Test from Kotest Plugin to Kotlin-test runtime #3193 (#3223) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor GuaranteeCaseTest to use kotlin test (#3226) Closes #3190 * refactor: migrate NotEmptySetTest to kotlin-test (#3230) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * refactor: migrate EagerEffectSpec to kotlin-test (#3233) Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor NullableSpec from Kotest Plugin to Kotlin-test runtime (#3236) #3153 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor BracketCaseTest to use kotlin test (#3237) Closes #3186 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Move arrow-functions tests to kotlin.test (#3243) * Inline `AtomicBoolean` (#3240) * Inline AtomicBoolean * Update API files --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * refactor: migrate MappersSpec to kotlin-test (#3248) * Refactor ResourceTestJvm from Kotest Plugin to Kotlin-test runtime (#3244) Closes #3213 * refactor: migrate FlowJvmTest to Kotlin-test (#3228) * Refactor ParZip9JvmTest from Kotest Plugin to Kotlin-test runtime (#3245) Closes #3211 * Refactor ParZip8JvmTest from Kotest Plugin to Kotlin-test runtime (#3246) Closes #3210 * refactor: migrate NumberInstancesTest to kotlin-test (#3232) * refactor: OptionTest to kotlin-test runtime (#3229) * Revert "Inline `AtomicBoolean` (#3240)" (#3279) This reverts commit a6f1e73. * Refactor ParZip6JvmTest from Kotest Plugin to Kotlin-test runtime (#3255) Closes #3208 * Refactor ParZip5JvmTest from Kotest Plugin to Kotlin-test runtime (#3256) Closes #3207 * Refactor ParZip3JvmTest from Kotest Plugin to Kotlin-test runtime (#3258) Closes #3205 * Refactor ParZip2JvmTest from Kotest Plugin to Kotlin-test runtime (#3259) Closes #3204 * Refactor ParMapJvmTest from Kotest Plugin to Kotlin-test runtime (#3260) Closes #3203 * Refactor ParZip4JvmTest from Kotest Plugin to Kotlin-test runtime (#3257) Closes #3206 * refactor: migrate RaiseAccumulateSpec to kotlin-test (#3250) * Refactor ParZip7JvmTest from Kotest Plugin to Kotlin-test runtime (#3247) Closes #3209 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update ComparisonKtTest.kt (#3274) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Update OptionSpec.kt (#3271) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Update TraceJvmSpec.kt (#3276) * Update TraceJvmSpec.kt * Update TraceJvmSpec.kt --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * Update ParZip9Test.kt (#3265) * Update ParZip9Test.kt * Update ParZip9Test.kt * Update ParZip8Test.kt (#3266) * Update ParZip7Test.kt (#3267) * Update ParZip6Test.kt (#3268) * Update ParZip5Test.kt (#3269) * Update ParZip4Test.kt (#3270) * Refactor CountDownLatchSpec and CyclicBarrierSpec to use kotlin test (#3227) * Refactor CountDownLatchSpec to use kotlin test Closes #3187 * Refactor CyclicBarrierSpec to use kotlin test Closes #3188 --------- Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Refactor NonEmptyListTest to kotlin-test (#3231) * Add kotlin test dependency * Refactor NonEmptyList Test to use kotlin test --------- Co-authored-by: Alejandro Serrano <trupill@gmail.com> * refactor: migrate EffectSpec to kotlin-test (#3234) * Refactor FlowTest to use kotlin test (#3238) Closes #3189 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * refactor: migrate IorSpec to kotlin-test (#3249) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * refactor: migrate ResultSpec to kotlin-test (#3251) Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> * refactor: migrate StructuredConcurrencySpec to kotlin-test (#3252) * refactor: migrate TraceSpec to kotlin-test (#3253) * refactor: migrate GeneratorsTest to kotlin-test (#3254) * Refactor RaceNJvmTest from Kotest Plugin to Kotlin-test runtime (#3261) Closes #3212 Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update ArrowResponseEAdapterTest.kt (#3264) * Update ArrowResponseEAdapterTest.kt * Update ArrowResponseEAdapterTest.kt --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update CollectionsSyntaxTests.kt (#3273) * Update CollectionsSyntaxTests.kt * Update CollectionsSyntaxTests.kt --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Alejandro Serrano <trupill@gmail.com> * Update NonFatalJvmTest.kt (#3277) * Update ArrowEitherCallAdapterTest.kt (#3278) * Update ArrowEitherCallAdapterTest.kt * Update ArrowEitherCallAdapterTest.kt * Move tests from `serialization` and `functions` completely to `kotlin.test` (#3289) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Fix problems with tests * Remove a bunch of warnings in `arrow-2` (#3282) Co-authored-by: serras <serras@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Finish transition to `kotlin.test` of `retrofit` and `fx-coroutines` (#3291) * Fix problems with concurrency in tests, take 8 * Port rest of `arrow-core` to `kotlin.test` (#3292) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Implement `fixedRate` using monotonic time source (#3294) Co-authored-by: serras <serras@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Port `optics` tests to `kotlin.test` (#3295) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Add or fix contracts in Raise (#3293) * Add or fix contracts in Raise * Make contracts less strict * Get back contract on Either * ignoreErrors should also be AT_MOST_ONCE --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Alternate `SingletonRaise` (#3328) Co-authored-by: Youssef Shoaib <canonballt@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: serras <serras@users.noreply.github.com> * Remove (unused) tests for high-arity module * Fix tests + Knit * Fix merge NullableSpec * Regression in Arb.list? * Fix test for nonEmptyList * Develocity warning * Fix merge problem with optics-ksp-plugin * Fix timeout in test --------- Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com> Co-authored-by: Sam Cooper <54266448+roomscape@users.noreply.github.com> Co-authored-by: Marc Moreno Ferrer <ignis.fatue@gmail.com> Co-authored-by: Javier Segovia Córdoba <7463564+JavierSegoviaCordoba@users.noreply.github.com> Co-authored-by: Alphonse Bendt <370821+abendt@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Andreas Storesund Madsen <andreas@asmadsen.no> Co-authored-by: molikuner <molikuner@gmail.com> Co-authored-by: Jonathan Lagneaux <lagneaux.j@gmail.com> Co-authored-by: Marcus Ilgner <mail@marcusilgner.com> Co-authored-by: Chris Black <2538545+chrsblck@users.noreply.github.com> Co-authored-by: Tejas Mate <hi.tejas@pm.me> Co-authored-by: HyunWoo Lee (Nunu Lee) <54518925+l2hyunwoo@users.noreply.github.com> Co-authored-by: serras <serras@users.noreply.github.com> Co-authored-by: Youssef Shoaib <canonballt@gmail.com>

nomisRev reviewed Jul 2, 2022

View reviewed changes

nomisRev requested a review from a team July 2, 2022 09:12

mjmoore force-pushed the 2743-UseDurationOverDouble branch 2 times, most recently from a5ed622 to bc06e65 Compare July 5, 2022 07:00

i-walker reviewed Jul 7, 2022

View reviewed changes

i-walker previously approved these changes Jul 7, 2022

View reviewed changes

nomisRev approved these changes Jul 7, 2022

View reviewed changes

mjmoore force-pushed the 2743-UseDurationOverDouble branch from 7d7c7e8 to 9c1dcc4 Compare July 7, 2022 10:00

i-walker self-requested a review July 7, 2022 14:34

nomisRev reviewed Jul 13, 2022

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/src/commonMain/kotlin/arrow/fx/coroutines/CircuitBreaker.kt Outdated Show resolved Hide resolved

serras mentioned this pull request Aug 9, 2022

Update API compatibility validator version #2794

Merged

nomisRev added arrow-fx Tickets belonging to Arrow Fx enhancement labels Aug 24, 2022

mjmoore force-pushed the 2743-UseDurationOverDouble branch from b70babd to d31cf29 Compare September 6, 2022 08:24

nomisRev reviewed Sep 8, 2022

View reviewed changes

arrow-libs/fx/arrow-fx-coroutines/api/arrow-fx-coroutines.api Show resolved Hide resolved

i-walker approved these changes Sep 8, 2022

View reviewed changes

mjmoore added 6 commits September 22, 2022 11:26

[2743] Migrate internal use of double to duration

85a63b0

[2743] Deprecate non duration factory method

f6e0b25

[2743] Update API defs

655849c

[2743] Adds validation tests for circuit breaker.

5b82e01

[2743] Validate non-zero durations.

4f943bd

[2743] Remove duplicated validation.

2e54c0f

mjmoore added 4 commits September 22, 2022 11:26

[2743] Test deprecated constuctor validation

74e6f6c

[2743] Fix variable in comments and error messages

6d10bfe

[2743] Fix binary compat

8fab978

[2743] Fix infinite recursion, update tests

d296cc9

mjmoore force-pushed the 2743-UseDurationOverDouble branch from 71b0e11 to d296cc9 Compare September 22, 2022 09:26

nomisRev approved these changes Sep 29, 2022

View reviewed changes

nomisRev merged commit 0c2de60 into arrow-kt:main Sep 29, 2022

renovate bot mentioned this pull request Nov 6, 2023

Update all dependencies (optics-setup) arrow-kt/Arrow-Maven-Template#29

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2743] Migrate internal use of CircuitBreaker double to duration #2748

[2743] Migrate internal use of CircuitBreaker double to duration #2748

mjmoore commented Jun 25, 2022

nomisRev commented Jun 27, 2022

mjmoore commented Jun 28, 2022

nomisRev left a comment •

edited

Loading

mjmoore commented Jul 5, 2022

i-walker Jul 7, 2022 •

edited

Loading

i-walker left a comment

nomisRev left a comment

nomisRev Jul 7, 2022

nomisRev commented Jul 16, 2022

nomisRev commented Jul 16, 2022

mjmoore commented Sep 6, 2022

nomisRev commented Sep 6, 2022

i-walker left a comment •

edited

Loading

mjmoore commented Sep 22, 2022

nomisRev commented Sep 22, 2022

nomisRev left a comment

[2743] Migrate internal use of CircuitBreaker double to duration #2748

[2743] Migrate internal use of CircuitBreaker double to duration #2748

Conversation

mjmoore commented Jun 25, 2022

nomisRev commented Jun 27, 2022

mjmoore commented Jun 28, 2022

nomisRev left a comment • edited Loading

Choose a reason for hiding this comment

mjmoore commented Jul 5, 2022

i-walker Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

i-walker left a comment

Choose a reason for hiding this comment

nomisRev left a comment

Choose a reason for hiding this comment

nomisRev Jul 7, 2022

Choose a reason for hiding this comment

nomisRev commented Jul 16, 2022

nomisRev commented Jul 16, 2022

mjmoore commented Sep 6, 2022

nomisRev commented Sep 6, 2022

i-walker left a comment • edited Loading

Choose a reason for hiding this comment

mjmoore commented Sep 22, 2022

nomisRev commented Sep 22, 2022

nomisRev left a comment

Choose a reason for hiding this comment

nomisRev left a comment •

edited

Loading

i-walker Jul 7, 2022 •

edited

Loading

i-walker left a comment •

edited

Loading