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

Clean up Jetbrains Backend plugin #6603

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Conversation

atduarte
Copy link
Contributor

@atduarte atduarte commented Nov 8, 2021

Description

  • Replace usage of environment variables, by using the Supervisor's endpoint Info.
  • Change ControllerStatusService to fail explicitly
  • Better error handling

Related Issue(s)

Fixes #6513

How to test

Validate heartbeats are still functioning.

Release Notes

NONE

@stale
Copy link

stale bot commented Nov 18, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Nov 18, 2021
@roboquat roboquat added size/L and removed size/M labels Nov 23, 2021
@stale stale bot removed meta: stale This issue/PR is stale and will be closed soon labels Nov 23, 2021
@atduarte atduarte force-pushed the ad/clean-up-backend-plugin branch from fe6901a to bb701cc Compare November 23, 2021 14:28
@atduarte atduarte marked this pull request as ready for review November 23, 2021 15:35
@atduarte
Copy link
Contributor Author

Before the IDE project starts, an error is logged:

2021-11-23 15:25:56,303 [   7812]  ERROR - rvices.ControllerStatusService - Failed to retrieve controller status. 
	...
Caused by: com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException: Instantiation of [simple type, class io.gitpod.ide.jetbrains.backend.services.ControllerStatusService$Response] value failed for JSON property projects due to missing (therefore NULL) value for creator parameter projects which is a non-nullable type
 at [Source: (String)"{
  "appPid": 997,
  "appVersion": "IU-213.5744.18",
  "runtimeVersion": "11_0_13b1751.19",
  "unattendedMode": true,
  "idePath": "/ide-desktop/backend"
}"; 
	...

@corneliusludmann do you think this is an issue? Do you have any suggestion?

@corneliusludmann
Copy link
Contributor

Before the IDE project starts, an error is logged:

do you think this is an issue? Do you have any suggestion?

When the project is not fully loaded the projects property is null. I think, we should gracefully handle this case and don't throw an error. Something like this:

diff --git a/components/ide/jetbrains/backend-plugin/src/main/kotlin/io/gitpod/ide/jetbrains/backend/services/ControllerStatusService.kt b/components/ide/jetbrains/backend-plugin/src/main/kotlin/io/gitpod/ide/jetbrains/backend/services/ControllerStatusService.kt
index ae99d3c43..5ecfaa12b 100644
--- a/components/ide/jetbrains/backend-plugin/src/main/kotlin/io/gitpod/ide/jetbrains/backend/services/ControllerStatusService.kt
+++ b/components/ide/jetbrains/backend-plugin/src/main/kotlin/io/gitpod/ide/jetbrains/backend/services/ControllerStatusService.kt
@@ -47,8 +47,8 @@ object ControllerStatusService {
                 throw IOException("Failed to retrieve controller status.", e)
             }
 
-            if (response.projects.isEmpty()) {
-                throw IOException("Failed to fetch controller status as project list is empty.")
+            if (response.projects.isNullOrEmpty()) {
+                return@retry ControllerStatus(false, 0);
             }
 
             ControllerStatus(
@@ -60,7 +60,7 @@ object ControllerStatusService {
     @JsonIgnoreProperties(ignoreUnknown = true)
     private data class Response(
         val appPid: Int,
-        val projects: List<Project>
+        val projects: List<Project>?
     ) {
         @JsonIgnoreProperties(ignoreUnknown = true)
         data class Project(

(the empty return ControllerStatus(false, 0) needs to be something sensible, though)

What do you think?

@atduarte
Copy link
Contributor Author

@corneliusludmann I think that's also reasonable. As we also have errors in the supervisor (?) while checking whether IDEA is up, I didn't consider it to much of an issue but in this case the returned value is actually "true".

@corneliusludmann corneliusludmann force-pushed the ad/clean-up-backend-plugin branch from 7fb89c5 to 5324b95 Compare November 29, 2021 10:34
@corneliusludmann
Copy link
Contributor

Looks great and works as expected. (Took the liberty to rebase the branch. 🙏)

/lgtm
/approve

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: b18dd01ad024dadd3045ab3c547ba2cd2f36f0dd

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: corneliusludmann

Associated issue: #6513

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 3c8b60c into main Nov 29, 2021
@roboquat roboquat deleted the ad/clean-up-backend-plugin branch November 29, 2021 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up the heartbeat Jetbrains's plugin
3 participants