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

Separate operator default implementation from library #303

Merged
merged 5 commits into from
May 23, 2024
Merged

Separate operator default implementation from library #303

merged 5 commits into from
May 23, 2024

Conversation

sgraband
Copy link
Contributor

This will help to ease customization of the operator.
For this a new default implementation was added (org.eclipse.theia.cloud.defaultoperator).
Also structured the library (org.eclipse.theia.cloud.operator) more into modules.
Add OperatorPlugin type:

  • Customizations can inject multiple operatorPlugins which will be started on startup.
  • Changed monitor activity tracking to a OperatorPlugin.

@sgraband
Separate operator default implementation from library
2f5ab6d 
This will help to ease customization of the operator.
For this a new default implementation was added (`org.eclipse.theia.cloud.defaultoperator`).
Also structured the library (`org.eclipse.theia.cloud.operator`) more into modules.
Add `OperatorPlugin` type:
- Customizations can inject multiple operatorPlugins which will be started on startup.
- Changed monitor activity tracking to a `OperatorPlugin`.
@sgraband sgraband requested a review from jfaltermeier May 15, 2024 08:54
jfaltermeier
jfaltermeier reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

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

Thanks, I've added a few first comments on the files.
I haven't tested it yet, but it looks very nice already

documentation/Architecture.md Outdated Show resolved Hide resolved
java/operator/org.eclipse.theia.cloud.operator/pom.xml Outdated Show resolved Hide resolved
java/operator/org.eclipse.theia.cloud.defaultoperator/Run Operator.launch Outdated Show resolved Hide resolved
java/operator/org.eclipse.theia.cloud.defaultoperator/Run Operator.launch Outdated Show resolved Hide resolved
java/operator/org.eclipse.theia.cloud.defaultoperator/Run Operator.launch Outdated Show resolved Hide resolved
...se.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/AbstractOperator.java Outdated Show resolved Hide resolved
...r/src/main/java/org/eclipse/theia/cloud/defaultoperator/DefaultTheiaCloudOperatorModule.java Outdated Show resolved Hide resolved
....cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/AbstractOperatorLauncher.java Outdated Show resolved Hide resolved
...ator/src/main/java/org/eclipse/theia/cloud/operator/di/AbstractTheiaCloudOperatorModule.java Outdated Show resolved Hide resolved
...eia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/util/JavaResourceUtil.java Outdated Show resolved Hide resolved
sgraband added 2 commits May 17, 2024 09:16
@sgraband
Address review feedback
4b7f269 
Fix `Run Operator` launch config.
Rename files consistently to `TheiaCloudOperatorX`.
Adjust documentation to make it more clear.
Add build and install launch configs for operator library.
@sgraband
Fix usage of theia.cloud and use Theia Cloud instead
84c376e
@sgraband sgraband requested a review from jfaltermeier May 17, 2024 07:39
@jfaltermeier
Copy link
Contributor

Thanks, I tested the basic startup use case and it was working.

Then I tried the monitor extension with the Theia extension, and this failed:

14:38:20.514 [pool-7-thread-1] INFO  org.eclipse.theia.cloud.operator.monitor.MonitorActivityTrackerImpl - [ws-theia-cloud-demo-foo-theia-cloud-io-session] Pinging session at 10.108.218.42
14:38:20.515 [pool-7-thread-1] INFO  org.eclipse.theia.cloud.operator.monitor.MonitorActivityTrackerImpl - [ws-theia-cloud-demo-foo-theia-cloud-io-session] GET http://10.108.218.42:3000/monitor/activity/lastActivity
14:38:20.518 [pool-7-thread-1] INFO  org.eclipse.theia.cloud.operator.monitor.MonitorActivityTrackerImpl - [ws-theia-cloud-demo-foo-theia-cloud-io-session] REQUEST FAILED (Returned 404: GET http://10.108.218.42:3000/monitor/activity/lastActivity
14:38:20.518 [pool-7-thread-1] INFO  org.eclipse.theia.cloud.operator.monitor.MonitorActivityTrackerImpl - [ws-theia-cloud-demo-foo-theia-cloud-io-session] Last reported activity was: 1970-01-01T00:00:00.000Z (28599278 minutes ago)
14:38:20.537 [pool-7-thread-1] INFO  org.eclipse.theia.cloud.operator.monitor.MonitorActivityTrackerImpl - [ws-theia-cloud-demo-foo-theia-cloud-io-session] Deleting session as timeout of 4 minutes was reached!

I am not sure if this is necessarily related to this PR, but I can't verify this before my vacation.
I've also rebuilt the sample demo applications, which might have introduced the issue.
It could also be a race condition, when the monitor extension is not yet responsible and we pick a 0 timestamp that leads to 1970-01-01T00:00:00.000Z instead of retrying later?
Could you maybe have a look at this as well to see of its related?

Otherwise from a general code refactoring POV I have no more comments and I think we may merge this if there are no regressions / the issue above is unrelated.

Maybe @lucas-koehler wants to have a short look as well at the library refactoring?

lucas-koehler
lucas-koehler reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

I had a brief look at the changes and they look pretty good to me ✨
I have only two minor notes left as comments inline.

documentation/Architecture.md Outdated Show resolved Hide resolved
...rator/src/main/java/org/eclipse/theia/cloud/operator/AbstractTheiaCloudOperatorLauncher.java Outdated Show resolved Hide resolved
@sgraband
Fix vscode monitor demo
9336190 
Built-in plugins now need to be extracted in the plugins folder.
Fix small typo.
Make AbstractTheiaCloudOperatorLauncher an interface and rename it.
@sgraband sgraband requested a review from lucas-koehler May 22, 2024 09:34
lucas-koehler
lucas-koehler reviewed May 22, 2024
View reviewed changes
Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. The monitor extension now also seems to work for me ✨
I have only three very minor suggestions left inline. Besides that, LGTM :)

...loud.operator/src/main/java/org/eclipse/theia/cloud/operator/TheiaCloudOperatorLauncher.java Outdated Show resolved Hide resolved
...loud.operator/src/main/java/org/eclipse/theia/cloud/operator/TheiaCloudOperatorLauncher.java Outdated Show resolved Hide resolved
...loud.operator/src/main/java/org/eclipse/theia/cloud/operator/TheiaCloudOperatorLauncher.java Outdated Show resolved Hide resolved
@sgraband sgraband requested a review from lucas-koehler May 22, 2024 14:00
lucas-koehler
lucas-koehler approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Thanks for the update and the great refactoring in general ✨ LGTM now!

@sgraband sgraband merged commit e805055 into main May 23, 2024
30 checks passed
@lucas-koehler lucas-koehler mentioned this pull request May 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucas-koehler lucas-koehler lucas-koehler approved these changes

@jfaltermeier jfaltermeier Awaiting requested review from jfaltermeier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sgraband @jfaltermeier @lucas-koehler