Cleanup and refactoring of REST Server #549

adamretter · 2015-03-31T00:39:36Z

Much cleanup and removal of duplicated code.
Easier to read code.
Fixed several black-hole bugs.
Made parameter handling consistent between http methods.
Made error handling in the REST interface consistent and correct.

dizzzz · 2015-03-31T07:27:55Z

Looks good.....

adamretter · 2015-03-31T08:43:36Z

@dizzzz Does that mean you will merge it?

ljo · 2015-03-31T09:16:21Z

Looks good, but I have not had the time to test it yet.

dizzzz · 2015-03-31T09:24:04Z

@adamretter we are a bit slow with pulling in PRs, since changes in 'essential' or 'core' areas need to be verifyied thoroughly (enough). I certainly do NOT want to slow down development, input from fellow devs like "looks good to me" (review) certainly helps to know who has looked into a PR.

I feel though that we need some talks -skype/hangout/...- to align who/how/when/what is pulled in, what is the criterium.

I fear that running the unit-tests is an essential step but not sufficient. But... what is then sufficient?

adamretter · 2015-03-31T09:28:19Z

@dizzzz Okay. I would like to get an idea if the next version is 2.3 or 3.0, certainly I have another 6 potential pull-requests stacked up on top of the ones I have sent. Some of them would be more suitable for a 3.0 as they are the removal of deprecated features. My personal feeling is the next version should be a 3.0, due to several things, but the major non-backwards change is the switch to Java 8.

Just let me know when you want to Skype?

Regards the unit tests, if you can't trust our test-suite then we need more tests!

ljo · 2015-03-31T09:41:25Z

Testing in a live system with additional extensions and xars enabled, using other than default configuration and with legacy or at least current production code I would say is preferred for central components like this on top of unit testing. So closer to integration/delivery/acceptance.

adamretter · 2015-03-31T09:50:09Z

@ljo But your live system is different to anyone else's live system, so it is not a repeatable test. For example none of my live systems use xars! Instead when issues are found by someone that are not covered by the test suite, then tests should be added, otherwise regressions are inevitable.

ljo · 2015-03-31T09:57:38Z

@adamretter yes, they should of course be fed back as unit tests. But to get a broad spectrum probe testing it in any one live system could add some insights even if not repeatable. This was mainly for @dizzzz on what could be done in addition. And I just said it was a preferred addition.

adamretter · 2015-03-31T09:58:15Z

@ljo okie doke.

wolfgangmm · 2015-03-31T10:30:43Z

Just for explanation: I test every pull request (or set thereof) which may affect core performance against two or three apps using a data set of around 20 gb. I mainly concentrate on install/upload performance, selected queries, memory consumption. This costs some time (about 2 to 3 hours), but gives me some confidence, in particular concerning memory leaks. I'm currently running the test for Adam's last PRs and I'm happy if Leif does the same.

Now that at least one of the data sets is actually in the public domain, I'd like to automate this type of load testing.

I would welcome a skype chat, also to coordinate preparations for the next release. There are still a lot of bugs to be addressed. How about tomorrow or Thursday evening?

dizzzz · 2015-03-31T10:32:57Z

Wednesday would be fine, thursday is not possible for me....

adamretter · 2015-03-31T10:56:14Z

@wolfgangmm Anytime is fine for me. I would also like to see those tests automated :-)

ljo · 2015-03-31T12:37:14Z

I just got a meeting moved to tomorrow evening, but let me know the time. From 20.30 CEST I will be available.

wolfgangmm · 2015-03-31T16:31:12Z

Ok, so let's say 20:30 CEST tomorrow. I'll send a hangout link.

shabanovd · 2015-03-31T16:40:43Z

any time is fine for me

dizzzz · 2015-04-01T11:37:32Z

Ok, great.

wolfgangmm · 2015-04-02T15:10:38Z

I was getting a deadlock after applying this PR to my test system. Investigating ...

adamretter · 2015-04-02T15:11:20Z

@wolfgangmm Okay very interesting. Let me know what you find...

wolfgangmm · 2015-04-04T09:41:30Z

The deadlock occurs because withCollection keeps a lock on the collection of the main query while parsing it. Import statements need to lock other resources in order to check their validity, so the collection should not remain locked.

As always, reproducing a deadlock depends on the exact timing, but it happens quite easily on my machine by hitting reload on the dashboard a few times.

Threads currently waiting for a lock:
=====================================
Thread: eXistThread-33
           Lock type: COLLECTION
           Lock mode: READ
             Lock id: /db/apps/dashboard/modules
             Held by: [eXistThread-45]
             Held by: [eXistThread-45]
             Held by: [eXistThread-45]
    Waiting for read: []
   Waiting for write: []

"eXistThread-45" #45 prio=5 os_prio=31 tid=0x00007fa455019000 nid=0x840b waiting for monitor entry [0x0000000125aa1000]
   java.lang.Thread.State: BLOCKED (on object monitor)
    at org.exist.storage.XQueryPool.returnObject(XQueryPool.java:133)
    - waiting to lock <0x000000078085fa00> (a org.exist.storage.XQueryPool)
    at org.exist.storage.XQueryPool.returnCompiledXQuery(XQueryPool.java:129)
    at org.exist.http.RESTServer.lambda$compileQuery$99(RESTServer.java:1403)
    at org.exist.http.RESTServer$$Lambda$51/1696695126.apply(Unknown Source)
    at org.exist.http.RESTServer.processExecutable(RESTServer.java:531)
    at org.exist.http.RESTServer.lambda$null$15(RESTServer.java:428)
    at org.exist.http.RESTServer$$Lambda$38/516271227.apply(Unknown Source)
    at java.util.Optional.map(Optional.java:215)
    at org.exist.http.RESTServer.lambda$processGetUri$16(RESTServer.java:425)
    at org.exist.http.RESTServer$$Lambda$32/1593647117.apply(Unknown Source)
    at org.exist.http.RESTServer.lambda$withDocument$89(RESTServer.java:1255)
    at org.exist.http.RESTServer$$Lambda$36/1231792430.apply(Unknown Source)
    at org.exist.http.RESTServer.lambda$null$86(RESTServer.java:1242)
    at org.exist.http.RESTServer$$Lambda$34/1980999548.apply(Unknown Source)
    at org.exist.http.RESTServer.lambda$withCollection$90(RESTServer.java:1280)
    at org.exist.http.RESTServer$$Lambda$33/675156664.apply(Unknown Source)
    at org.exist.http.RESTServer.lambda$withDocument$87(RESTServer.java:1238)
    at org.exist.http.RESTServer$$Lambda$31/1717034254.apply(Unknown Source)
    at org.exist.http.RESTServer.processGetUri(RESTServer.java:424)
    at org.exist.http.RESTServer.doGet(RESTServer.java:381)

"eXistThread-33" #33 prio=5 os_prio=31 tid=0x00007fa457a1e800 nid=0x7503 in Object.wait() [0x00000001292b8000]
   java.lang.Thread.State: TIMED_WAITING (on object monitor)
    at java.lang.Object.wait(Native Method)
    at org.exist.storage.lock.ReentrantReadWriteLock.acquire(ReentrantReadWriteLock.java:148)
    - locked <0x0000000780f03d70> (a org.exist.storage.lock.ReentrantReadWriteLock)
    at org.exist.collections.Collection.getDocumentWithLock(Collection.java:789)
    at org.exist.storage.NativeBroker.getXMLResource(NativeBroker.java:2181)
    at org.exist.source.DBSource.isValid(DBSource.java:89)
    at org.exist.xquery.ExternalModuleImpl.moduleIsValid(ExternalModuleImpl.java:252)
    at org.exist.xquery.XQueryContext.checkModulesValid(XQueryContext.java:1592)
    at org.exist.xquery.PathExpr.isValid(PathExpr.java:492)
    at org.exist.storage.XQueryPool.borrowObject(XQueryPool.java:195)
    at org.exist.storage.XQueryPool.borrowCompiledXQuery(XQueryPool.java:205)
    - locked <0x000000078085fa00> (a org.exist.storage.XQueryPool)
    at org.exist.http.urlrewrite.XQueryURLRewrite.runQuery(XQueryURLRewrite.java:681)

shabanovd · 2015-04-04T10:04:43Z

Think that

if (!query.isValid()) {
    remove(key);
    return null;
}

must be done outside synchronized block and borrowCompiledXQuery should be none synchronized

adamretter · 2015-04-04T11:53:15Z

Thanks @wolfgangmm I will take a look at this in the next 2 days or so (playing with transactions at the moment) and also check @shabanovd suggestion.

duncdrum · 2018-08-01T10:51:45Z

@adamretter with new interest in the REST api from @eXist-db/tools maybe its a good time to rebase this for the next RC?

duncdrum · 2019-03-27T23:06:46Z

So…?

adamretter · 2019-03-28T03:06:14Z

It is unlikely I will revive this for eXist-db. FusionDB will have a new http server to replace this. Unless anyone wants to continue this work, I suggest we close the PR

duncdrum · 2019-03-28T07:13:00Z

well @line-o and i talked about update to REST, @adamretter could you rebase this, so we see where its at?

adamretter · 2019-03-28T07:28:26Z

@duncdrum sorry, rebasing this requires resolving a lot of merge conflicts. Someone else is welcome to do so though...

line-o · 2019-03-28T15:55:02Z

@adamretter We would need to understand the changes and concepts behind this, in order to be able to do revive development. What are the expected benefits that would justify this?

adamretter · 2019-03-29T03:25:15Z

@line-o there are as laid out in the bullet points at the top of this PR.

adamretter · 2021-02-01T19:09:46Z

I won't be developing this further. If someone else wishes to continue with it, please feel free.

adamretter added bug issue confirmed as bug enhancement new features, suggestions, etc. labels Mar 31, 2015

adamretter added this to the eXist-2.3 milestone Mar 31, 2015

adamretter mentioned this pull request Mar 31, 2015

Remove URLDecoder call from path parsing #127

Closed

adamretter modified the milestones: eXist-3.0, eXist-2.3 Apr 1, 2015

adamretter mentioned this pull request Apr 2, 2015

ARM (Automatic Resource Management) #557

Open

adamretter force-pushed the restserver branch 2 times, most recently from b76a807 to 9439e6e Compare April 10, 2015 16:34

adamretter removed this from the eXist-3.3.0 milestone Jul 3, 2017

adamretter modified the milestones: eXist-3.4.0, eXist-3.5.0 Jul 27, 2017

adamretter modified the milestones: eXist-3.5.0, eXist-3.6.0 Sep 28, 2017

adamretter modified the milestones: eXist-3.6.0, eXist-3.7.0 Nov 27, 2017

adamretter added the in progress Issue is actively being worked upon label Jan 24, 2018

adamretter modified the milestones: eXist-3.7.0, eXist-4.1.0 Feb 14, 2018

wolfgangmm modified the milestones: eXist-4.1.0, eXist-5.0.0 Apr 16, 2018

adamretter modified the milestones: eXist-5.1.0, eXist-5.2.0 Nov 19, 2019

adamretter modified the milestones: eXist-5.2.0, eXist-5.3.0 Jan 23, 2020

line-o marked this pull request as draft April 15, 2020 06:58

line-o removed the in progress Issue is actively being worked upon label Nov 12, 2020

line-o removed this from the eXist-5.3.0 milestone Nov 12, 2020

adamretter closed this Feb 1, 2021

Uh oh!

Cleanup and refactoring of REST Server #549

Cleanup and refactoring of REST Server #549

Uh oh!

Conversation

adamretter commented Mar 31, 2015

Uh oh!

dizzzz commented Mar 31, 2015

Uh oh!

adamretter commented Mar 31, 2015

Uh oh!

ljo commented Mar 31, 2015

Uh oh!

dizzzz commented Mar 31, 2015

Uh oh!

adamretter commented Mar 31, 2015

Uh oh!

ljo commented Mar 31, 2015

Uh oh!

adamretter commented Mar 31, 2015

Uh oh!

ljo commented Mar 31, 2015

Uh oh!

adamretter commented Mar 31, 2015

Uh oh!

wolfgangmm commented Mar 31, 2015

Uh oh!

dizzzz commented Mar 31, 2015 via email

Uh oh!

adamretter commented Mar 31, 2015

Uh oh!

ljo commented Mar 31, 2015

Uh oh!

wolfgangmm commented Mar 31, 2015

Uh oh!

shabanovd commented Mar 31, 2015

Uh oh!

dizzzz commented Apr 1, 2015

Uh oh!

wolfgangmm commented Apr 2, 2015

Uh oh!

adamretter commented Apr 2, 2015

Uh oh!

wolfgangmm commented Apr 4, 2015

Uh oh!

shabanovd commented Apr 4, 2015

Uh oh!

adamretter commented Apr 4, 2015

Uh oh!

duncdrum commented Aug 1, 2018

Uh oh!

duncdrum commented Mar 27, 2019

Uh oh!

adamretter commented Mar 28, 2019

Uh oh!

duncdrum commented Mar 28, 2019

Uh oh!

adamretter commented Mar 28, 2019

Uh oh!

line-o commented Mar 28, 2019

Uh oh!

adamretter commented Mar 29, 2019

Uh oh!

adamretter commented Feb 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants