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.

Sign up for GitHub

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

Jump to bottom

Better handling for broken connection to Foojay backend #47

Closed
jamesward opened this issue Sep 8, 2023 · 11 comments
Closed

Better handling for broken connection to Foojay backend #47

jamesward opened this issue Sep 8, 2023 · 11 comments

Comments

@jamesward
Copy link 

* What went wrong:
Failed to query the value of property 'buildFlowServiceProperty'.
> Could not isolate value org.jetbrains.kotlin.gradle.plugin.statistics.BuildFlowService$Parameters_Decorated@1612c9e7 of type BuildFlowService.Parameters
   > A problem occurred configuring project ':dev'.
      > Failed to calculate the value of task ':dev:compileJava' property 'javaCompiler'.
         > Requesting vendor list failed: <html>
           <head><title>503 Service Temporarily Unavailable</title></head>
           <body>
           <center><h1>503 Service Temporarily Unavailable</h1></center>
           <hr><center>nginx</center>
           </body>
           </html>

Possible future enhancements: Better error message. Fallback to something else.
@tinder-ryantrontz
Copy link

tinder-ryantrontz commented Sep 8, 2023 •
edited
Loading

Hey there, I'm here because of the same error. I may not have a clear understanding of the purpose of this plugin, but if we're running a build in docker container like eclipse-temurin, why do we need to resolve the toolchain or compiler from the DiscoAPI at all if it's definitely already present?

@msfjarvis
Copy link

FYI it's related to upstream downtime with foojay.io, being tracked at foojayio/discoapi#85

@geertjanw
Copy link

Back online now.

@StefMa
Copy link
Contributor

StefMa commented Sep 9, 2023

The "problem" within the plugin is, that it made always connection to api.foojay.io/disco/v3.0/distributions to find the URL for the maybe to download JDK.
The logic, if the JDK has to be downloaded is part of the core gradle/gradle implementation somewhere.

Currently, in case the api call doesn't succeed, this plugin throws an exception.
Maybe it make sense to return an Optional.empty instead but log this as an... Warning? 🤔

In this case it could be possible that:
In case it is down again, but the jdk is already available locally, everything still works.
In case it is down again, but the jdk is not available, the warning will be printed...

Hmm.. 🤷‍♂️ 😂

@StefMa
Copy link
Contributor

StefMa commented Sep 9, 2023

Just thought about this again.
I guess it is really the case that:
Those resolvers provide only URLs to the matching jdks.
The build system itself while determinate if it will uses the url or not (because it is already installed locally).

Therefore resolvers like this plugin don't have the information if a url should be provided or not. It just does it... Right? 🤔

So what is gradle doing?
It collects all resolvers, get the URLs and check later if it has to use the url or not.

Maybe this logic should change. It should first check if the jdk is already available and afterwards ask the resolver to provide the url (in case it is not available locally)...

Not sure if Iam correct with my assumptions 😂

@jbartok jbartok changed the title Something is down Better handling for broken connection to Foojay backend Sep 12, 2023
@Virtlink Virtlink mentioned this issue Sep 21, 2023
Open
@tr00gle
Copy link

tr00gle commented Feb 14, 2024

The API went down again today -- foojayio/discoapi#96

This PR, in particular @StefMa's ideas on how the logic should work, would be really impactful. The API is up perfectly nearly all of the time, but when it's not, all gradle builds fail. I think the community would appreciate a way to leave out hitting the API and still use toolchains, especially when it's a certainty that the desired JDK is available locally.

@jbartok
Copy link
Member

jbartok commented Feb 19, 2024

Hey there, I'm here because of the same error. I may not have a clear understanding of the purpose of this plugin, but if we're running a build in docker container like eclipse-temurin, why do we need to resolve the toolchain or compiler from the DiscoAPI at all if it's definitely already present?

The purpose of this plugin is to facilitate the download of a java toolchain IF you don't already have one locally. Gradle will not download anything if it can find a local one. If you think you have a JDK/JRE locally and you still see this plugin in action, then you have the local ones misconfigured somehow. See here: https://docs.gradle.org/current/userguide/toolchains.html#sec:auto_detection

@jbartok
Copy link
Member

jbartok commented Feb 19, 2024

Just thought about this again. I guess it is really the case that: Those resolvers provide only URLs to the matching jdks. The build system itself while determinate if it will uses the url or not (because it is already installed locally).

Therefore resolvers like this plugin don't have the information if a url should be provided or not. It just does it... Right? 🤔

So what is gradle doing? It collects all resolvers, get the URLs and check later if it has to use the url or not.

Maybe this logic should change. It should first check if the jdk is already available and afterwards ask the resolver to provide the url (in case it is not available locally)...

Not sure if Iam correct with my assumptions 😂

You're wrong with your assumptions. If there are locally available matching JDK/JREs, then no downloading/connecting to Foojay will take place. This is core Gradle logic, see https://github.com/gradle/gradle/blob/1e9501765e70a9dab7604ef58a3fef8d7a27e390/platforms/jvm/toolchains-jvm/src/main/java/org/gradle/jvm/toolchain/internal/JavaToolchainQueryService.java

@jbartok
Copy link
Member

jbartok commented Feb 19, 2024 •
edited
Loading

Currently, in case the api call doesn't succeed, this plugin throws an exception. Maybe it make sense to return an Optional.empty instead but log this as an... Warning? 🤔

This sound like a good idea, contributions welcome.

@StefMa StefMa mentioned this issue Feb 20, 2024
Closed
@StefMa
Copy link
Contributor

StefMa commented Feb 20, 2024

Thanks for the feedback @jbartok !
You're right. I tested a bit more and indeed, this plugin will not called in case something is installed locally 👍

I create a PR here to "enhance" the error message... and a bit more 😁

@jbartok
Copy link
Member

jbartok commented Feb 27, 2024

I'm closing this issue, discussions on the reason can be found in the PR's comments.

@jbartok jbartok closed this as completed Feb 27, 2024
@jbartok jbartok mentioned this issue Feb 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Better api error handling StefMa/foojay-toolchains
7 participants
@jamesward @geertjanw @jbartok @StefMa @tr00gle @msfjarvis @tinder-ryantrontz