Skip to content

Pass test time estimate arg to stabilize smart flank without cached results#635

Merged
bootstraponline merged 1 commit into
Flank:masterfrom
RainNapper:master
Feb 26, 2020
Merged

Pass test time estimate arg to stabilize smart flank without cached results#635
bootstraponline merged 1 commit into
Flank:masterfrom
RainNapper:master

Conversation

@RainNapper
Copy link
Copy Markdown
Contributor

@RainNapper RainNapper commented Feb 4, 2020

Context: #632

Ran into some local issues running all of the tests (stacktrace below[1]), but I think I covered most if not all of the places that this change touches.

Please leave comments on what I missed, or what I need to get a local build working. I can run unit tests in Android Studio, but hit some issues[2] running ./gradlew build

[1]

java.lang.VerifyError: Stack map does not match the one at exception handler 77
Exception Details:
  Location:
    com/fasterxml/jackson/databind/deser/std/StdDeserializer._parseDate(Lcom/fasterxml/jackson/core/JsonParser;Lcom/fasterxml/jackson/databind/DeserializationContext;)Ljava/util/Date; @77: astore
  Reason:
    Type 'com/fasterxml/jackson/core/JsonParseException' (current frame, stack[0]) is not assignable to 'com/fasterxml/jackson/core/exc/StreamReadException' (stack map, stack[0])

[2]

ftl.args.GcloudYmlTest > initializationError FAILED
    java.lang.NoClassDefFoundError: Could not initialize class ftl.test.util.FlankTestRunner

@bootstraponline
Copy link
Copy Markdown
Contributor

Thank you for the detailed pull request and issue! I will take a look when I have some time.

@bootstraponline
Copy link
Copy Markdown
Contributor

re: #632

Option 1 - Make the 10 second default significantly higher (2-5m)
Having an over-estimated default means that we err on the side of too many shards rather than too few. This effectively trades cost for stability. Enabling smart flank will mitigate this cost.

I prefer option 1 over introducing another flag. 10s was randomly chosen since it seemed to work well. Updating the default to something like 2m probably makes the most sense.

Flank already has a ton of config options. I am hesitant to add more unless we truly have a need.

@RainNapper
Copy link
Copy Markdown
Contributor Author

re: #632

Option 1 - Make the 10 second default significantly higher (2-5m)
Having an over-estimated default means that we err on the side of too many shards rather than too few. This effectively trades cost for stability. Enabling smart flank will mitigate this cost.

I prefer option 1 over introducing another flag. 10s was randomly chosen since it seemed to work well. Updating the default to something like 2m probably makes the most sense.

Flank already has a ton of config options. I am hesitant to add more unless we truly have a need.

That sounds good to me. Simplifies the change too. I went with 2m.

@bootstraponline
Copy link
Copy Markdown
Contributor

Thanks! That looks great.

I have to setup the bitrise jobs + CLA bot since we moved to the new Flank org. After that I'll merge this PR.

@bootstraponline
Copy link
Copy Markdown
Contributor

Hi, sorry for the delay! We now have 2 full time resources working on Flank. 😄

I've merged the pull request.

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.

2 participants