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

docgen: bump railroad diagram generation timeout to 120s #64430

Merged

Conversation

RichardJCai
Copy link
Contributor

@RichardJCai RichardJCai commented Apr 29, 2021

Some diagrams are too long to be generated in the current 3s. Bumping up to 120s for now since generating diagrams isn't exactly time sensitive.

Release note: None

@RichardJCai RichardJCai requested review from ianjevans, ericharmeling and a team April 29, 2021 20:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

I actually thought this timeout limit was set on the server side... This is really nice. I've been using a local JAR to build the larger files (something to also consider, for stability).

@RichardJCai
Copy link
Contributor Author

I actually thought this timeout limit was set on the server side... This is really nice. I've been using a local JAR to build the larger files (something to also consider, for stability).

Yeah I prefer to use the JAR as well for stability but currently docgen will fail on our generated bnfs without a JAR. Figure its beneficial to also get this working.

@@ -77,7 +78,8 @@ func GenerateRRNet(bnf []byte) ([]byte, error) {
v.Add("options", "factoring")
v.Add("options", "inline")

resp, err := httputil.Post(context.TODO(), rrAddr, "application/x-www-form-urlencoded", strings.NewReader(v.Encode()))
httpClient := httputil.NewClientWithTimeout(120 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

if you want something fancy you should make this a flag with flag.Duration :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call I think it's pretty straight forward to do this as well. Will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PTAL @otan

@RichardJCai RichardJCai force-pushed the change_diagram_gen_timeout_04292021 branch from 49b83cb to 04f8ac3 Compare April 30, 2021 16:54
@blathers-crl blathers-crl bot requested a review from otan April 30, 2021 16:54
Previously it was hardcoded to 3s timeout when using the HTTP api for
either railroad generation or bnf generation.

Now the user can pass a value via the --timeout flag to set a
timeout for the HTTP apis.

Default is 120s.

Release note: None
@RichardJCai RichardJCai force-pushed the change_diagram_gen_timeout_04292021 branch from 04f8ac3 to dfcd8b7 Compare April 30, 2021 17:00
@RichardJCai
Copy link
Contributor Author

Thanks for the reviews!
bors r+

@craig
Copy link
Contributor

craig bot commented May 6, 2021

Build succeeded:

@craig craig bot merged commit dee0558 into cockroachdb:master May 6, 2021
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.

5 participants