Make it possible to have distributed ProjectRegistry #9848

gazarenkov · 2018-05-29T10:32:17Z

What does this PR do?

Remake ProjectRegistry as interface and make current (inmemory) registry as default implementation
Create RegisteredProject as interface (moved to shared package) with corresponding DTO
Create experimental PersistingProjectRegistry implementation which caches current project state in the shared Project storage (under //.che/tmp/projectConfig directory)
RootDirPathProvider instead of direct using property for flexibility (caused a lot of changes in code)

What issues does this PR fix or reference?

as stated in the title

Docs PR

n/a yet

# Conflicts: # ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/context/AppContextImpl.java

…roject' volume defined

…re no 'project' volume defined" This reverts commit 127a79e.

…roject' volume defined

…ProjectType

…ValueProvider

…ode, remove highlighter from the source folder field

vparfonov · 2018-05-29T15:33:35Z

...pi-project/src/test/java/org/eclipse/che/api/watcher/server/impl/FileWatcherManagerTest.java

@@ -84,4 +86,11 @@ public void shouldUnWatchByMatcher() throws Exception {

    verify(fileWatcherByPathMatcher).unwatch(ID);
  }
+
+  private static class DummyRootProvider extends RootDirPathProvider {


Here will be better use Mock instead of Dummy implementation, this is allow avoid duplication

not sure it simplifies a lot but fixed, thanks )

vparfonov · 2018-05-29T15:34:45Z

ci-test

codenvy-ci · 2018-05-29T16:03:19Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9848
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

skabashnyuk · 2018-05-30T06:40:51Z

...jdt/org-eclipse-core-resources/src/main/java/org/eclipse/core/resources/ResourcesPlugin.java

@@ -189,12 +190,12 @@
  @Inject
  public ResourcesPlugin(
      @Named("che.jdt.workspace.index.dir") String indexPath,
-      @Named("che.user.workspaces.storage") String workspacePath,
+      RootDirPathProvider pathProvider,


what if keep in constructor
@Named("che.workspaces.storage") String workspacePath
and will use provider for this property

bind(String.class) .annotatedWith(Names.named("che.workspaces.root.dir")) .toProvider(RootDirPathProvider.class);

why to make this logic more complex?

what needed is to inject configuration object (called RootDirPathProvider)?

If you do not mind, I'll put in my five cents. Though technically there is almost no difference between two cases, I find it more comfortable to remember, to navigate and to use (in IDE) RootDirPathProvider class instead of a variable named "che.workspaces.root.dir" or "che.workspace.storage", it's much easier to access provider class (or property) documentation and so on. Probably it is caused by the way object oriented programmer thinks - in objects and classes, not properties and values.

skabashnyuk · 2018-05-30T06:42:22Z

...project/src/main/java/org/eclipse/che/api/project/server/impl/PersistingProjectRegistry.java

+import org.eclipse.che.api.project.shared.RegisteredProject;
+import org.eclipse.che.api.project.shared.dto.RegisteredProjectDto;
+import org.eclipse.che.dto.server.DtoFactory;
+


as I said it is still experimental impl (kinda placeholder) and not bound yet, of course javadoc will be added

skabashnyuk · 2018-05-30T06:43:21Z

...project/src/main/java/org/eclipse/che/api/project/server/impl/PersistingProjectRegistry.java

+                DtoFactory.getInstance().createDtoFromJson(json, RegisteredProjectDto.class));
+          }
+        } catch (Exception e) {
+          e.printStackTrace();


LOG.error ?

ok, will do

skabashnyuk · 2018-05-30T06:47:35Z

...e-api-project/src/main/java/org/eclipse/che/api/project/server/impl/RootDirPathProvider.java

+
+  @Inject(optional = true)
+  @Named("che.user.workspaces.storage")
+  protected File rootFile = null;


if you will use constructor injection then you can reuse it in tests.

well, I do not see any blocker for tests, object just need to be prepared according to the contract.

skabashnyuk · 2018-05-30T06:51:11Z

[INFO] Processed 49 files (1 non-complying).
[ERROR] Found 1 non-complying files, failing build
[ERROR] Non complying file: /home/codenvy/workspace/che-pullrequests-test-ocp/plugins/plugin-maven/che-plugin-maven-server/src/main/java/org/eclipse/che/plugin/maven/server/rest/MavenServerService.java
[JENKINS] Archiving disabled

please run mvn fmt:format

gazarenkov · 2018-05-30T07:13:24Z

@skabashnyuk ok, sorry for format but that's what I edited with GH, probably was not a good idea though...
BTW, I do not think it is reasonable to ask for reformatting just because there are 4 spaces in empty line (added by editor) - I would say we need to check with formatter whether it can be more reasonable.
WDYT?

skabashnyuk · 2018-05-30T07:20:30Z

BTW, I do not think it is reasonable to ask for reformatting just because there are 4 spaces in empty line (added by editor) - I would say we need to check with formatter whether it can be more reasonable.
WDYT?

I'm not sure that issue is only in spaces. I would recommend configuring your ide to use google's formatted and space settings.

gazarenkov · 2018-05-30T07:22:28Z

I compared 2 files before commented and yes, only spaces and as I said I used GH editor and what I did is just removing unused comments.

skabashnyuk · 2018-05-30T07:33:51Z

We can't run selenium test until we have formatting issue. Maven can't compile sources. It has to be fixed somehow.

RomanNikitenko · 2018-05-30T07:37:12Z

...-api-project/src/main/java/org/eclipse/che/api/project/server/impl/ProjectImportManager.java

@@ -318,8 +319,7 @@ private RegisteredProject doImportInternally(
        projectConfigRegistry.put(newProjectConfig, true, false);
      }

-      return projectConfigRegistry
-          .get(wsPath)
+      return Optional.ofNullable(projectConfigRegistry.getOrNull(wsPath))


You can use return projectConfigRegistry.get(wsPath) here if I'm not mistaken, but up to you

@RomanNikitenko I guess you are right, just considered it as risky at that time :)
if you or @dkuleshov think it is totally safe - will fix it immediatelly
WDYT?

I guess we can safely use projectConfigRegistry.get(wsPath) since it already returns Optional.

gazarenkov · 2018-05-30T07:37:31Z

@skabashnyuk sure, fixed alltogether

RomanNikitenko · 2018-05-30T07:38:57Z

...e-api-project/src/main/java/org/eclipse/che/api/project/server/impl/RootDirPathProvider.java

+/**
+ * Provides path for projects root directory using: env variable 'CHE_PROJECTS_ROOT' (which is set
+ * by workspace API == "project" volume configured) if not - uses 'che.user.workspaces.storage'
+ * property otherwise - default with '/project' directory (backward comptible solution)


typo comptible - compatible

fair enough :) thanks, fixed

tolusha · 2018-05-30T07:50:24Z

...project/src/main/java/org/eclipse/che/api/project/server/impl/PersistingProjectRegistry.java

+
+  private final RegisteredProjectFactory registeredProjectFactory;
+  private final FsManager fsManager;
+  private final FileCache cache;


I would say it isn't a cache but rather a persistence manager.
Beside this I would add cache (basically a map) not to access the fs for every get operations.

@tolusha well, we have this kind of map in default (inmemory, local) registry but an idea is just opposite - to simply reuse a projects volume as a shared for all the machines place (it should be by nature). So it call shared data all the time, not a local map.

So, I'd consider it as a (persisted, not optimized by performance) data cache.

As I said it is experimental and one of the reason is to also learn how it might impact on overall performance. May be it will be OK with this implementation, maybe we can reduce amount of calls, maybe it is not acceptable at all, I do not know yet and would not like to start with complex (data grid like solution) either.

Does it make sense?

This makes sense for me, thx.

musienko-maxim · 2018-05-30T11:04:14Z

ci-test

codenvy-ci · 2018-05-30T16:17:08Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9848
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

musienko-maxim · 2018-05-31T11:40:44Z

In the functionality that is covered by selenium tests we did not find new regression. We have 4 failed tests which failed in the unexpected place but after rerun they passed as well.

* Adding pipeline * del jenkinsfile * Make exec agent not to use setsid for other than Linux envs * revert pom.xml * goformat * get projects folder from workkspace configuration * Merge remote-tracking branch 'upstream/master' * clean code * clean code * Fix calculation Projects Root so it wont cause NPE if there are no 'project' volume defined * Revert "Fix calculation Projects Root so it wont cause NPE if there are no 'project' volume defined" This reverts commit 127a79e. * Fix calculation Projects Root so it wont cause NPE if there are no 'project' volume defined * replace provided attribute values with stored (src, out) in PlainJavaProjectType * fix PlainJavaProjectType for getting rid and deprecate using SettableValueProvider * remove comented code * hide brouse source folder button, set source folder field as disabled * hide the browse source button, set source folder field to read only mode, remove highlighter from the source folder field * adapt test for current changes on UI, set save button to enable state * apply formatting * fs cache PT * fix order of steps in the test * Refactor RegisteredProject to make it possible to cache it (as DTO) to local file * cache Projects and PTs to files * Make it possible to have several impls for ProjectRegistry preparing it to be distributed across multi machines env * merge upstream * merge upstream * merge upstream * Update MavenServerService.java * clean code * small fix * small fix * small fix

gazarenkov and others added 30 commits February 25, 2018 17:33

Adding pipeline

279a8f4

Merge remote-tracking branch 'upstream/master'

75110a8

del jenkinsfile

303f689

Merge remote-tracking branch 'upstream/master'

f9e5815

Make exec agent not to use setsid for other than Linux envs

6fa7a55

revert pom.xml

714aa1c

goformat

d778a01

get projects folder from workkspace configuration

95ee1cc

Merge remote-tracking branch 'upstream/master'

763848a

# Conflicts: # ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/context/AppContextImpl.java

Merge remote-tracking branch 'upstream/master'

241390d

Merge with upstream

d625790

clean code

99abb5e

clean code

90b9167

Fix calculation Projects Root so it wont cause NPE if there are no 'p…

127a79e

…roject' volume defined

Revert "Fix calculation Projects Root so it wont cause NPE if there a…

2b5c11c

…re no 'project' volume defined" This reverts commit 127a79e.

Merge remote-tracking branch 'upstream/master'

f69fd75

Fix calculation Projects Root so it wont cause NPE if there are no 'p…

1a407c0

…roject' volume defined

Merge remote-tracking branch 'upstream/master'

4fff97c

replace provided attribute values with stored (src, out) in PlainJava…

47faa53

…ProjectType

Merge remote-tracking branch 'upstream/master'

e676928

fix PlainJavaProjectType for getting rid and deprecate using Settable…

82e4c17

…ValueProvider

remove comented code

3232ecc

hide brouse source folder button, set source folder field as disabled

65238e2

hide the browse source button, set source folder field to read only m…

e6414e9

…ode, remove highlighter from the source folder field

adapt test for current changes on UI, set save button to enable state

d368a0d

apply formatting

7e56901

fs cache PT

63eaf6e

merge upstream

1eb3cb7

Merge branch 'master' of https://github.com/gazarenkov/che

cd193c0

fix order of steps in the test

32f0767

vparfonov reviewed May 29, 2018

View reviewed changes

clean code

3a680bc

skabashnyuk reviewed May 30, 2018

View reviewed changes

small fix

56377b8

RomanNikitenko reviewed May 30, 2018

View reviewed changes

small fix

7a67aa9

tolusha reviewed May 30, 2018

View reviewed changes

RomanNikitenko approved these changes May 30, 2018

View reviewed changes

small fix

6266a5d

tolusha approved these changes May 30, 2018

View reviewed changes

skabashnyuk approved these changes May 30, 2018

View reviewed changes

vparfonov approved these changes May 31, 2018

View reviewed changes

musienko-maxim approved these changes May 31, 2018

View reviewed changes

gazarenkov merged commit 00ca159 into eclipse-che:master May 31, 2018

gazarenkov removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label May 31, 2018

benoitf added this to the 6.7.0 milestone Jun 4, 2018

RomanNikitenko mentioned this pull request Jun 12, 2018

Delete only child projects from project config registry at removing project #10005

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it possible to have distributed ProjectRegistry #9848

Make it possible to have distributed ProjectRegistry #9848

gazarenkov commented May 29, 2018

vparfonov May 29, 2018

gazarenkov May 29, 2018

vparfonov commented May 29, 2018

codenvy-ci commented May 29, 2018

skabashnyuk May 30, 2018

gazarenkov May 30, 2018

dkuleshov May 30, 2018

skabashnyuk May 30, 2018

skabashnyuk May 30, 2018

gazarenkov May 30, 2018

skabashnyuk May 30, 2018

gazarenkov May 30, 2018

skabashnyuk May 30, 2018

gazarenkov May 30, 2018

skabashnyuk commented May 30, 2018

gazarenkov commented May 30, 2018

skabashnyuk commented May 30, 2018

gazarenkov commented May 30, 2018

skabashnyuk commented May 30, 2018

RomanNikitenko May 30, 2018

gazarenkov May 30, 2018

dkuleshov May 30, 2018 •

edited

Loading

gazarenkov May 30, 2018

gazarenkov commented May 30, 2018

RomanNikitenko May 30, 2018

gazarenkov May 30, 2018

tolusha May 30, 2018

gazarenkov May 30, 2018

tolusha May 30, 2018

musienko-maxim commented May 30, 2018

codenvy-ci commented May 30, 2018

musienko-maxim commented May 31, 2018

Make it possible to have distributed ProjectRegistry #9848

Make it possible to have distributed ProjectRegistry #9848

Conversation

gazarenkov commented May 29, 2018

What does this PR do?

What issues does this PR fix or reference?

Docs PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vparfonov commented May 29, 2018

codenvy-ci commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk commented May 30, 2018

gazarenkov commented May 30, 2018

skabashnyuk commented May 30, 2018

gazarenkov commented May 30, 2018

skabashnyuk commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkuleshov May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gazarenkov commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musienko-maxim commented May 30, 2018

codenvy-ci commented May 30, 2018

musienko-maxim commented May 31, 2018

dkuleshov May 30, 2018 •

edited

Loading