[Geoserver-devel] some control-flow improvements

GeoServer GeoServer Developer
1

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow&gt;
Let me know if that seems good to commit.

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

2

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com…> wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow>
Let me know if that seems good to commit.

I’m buried right now, but I’ll have a look as soon as I can (the weekend at
worst)

Cheers
Andrea

–

Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

3

On Thu, Mar 22, 2012 at 10:54 AM, Andrea Aime
<andrea.aime@anonymised.com> wrote:

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow&gt;
Let me know if that seems good to commit.

I'm buried right now, but I'll have a look as soon as I can (the weekend at
worst)

Sounds good, thanks.

Gabriel

Cheers
Andrea

--
-------------------------------------------------------
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

-------------------------------------------------------

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

4

Btw, I see now that SingleIpFlowController should rather extend
SingleQueueFlowController, so take that for granted, just no time to
update the patch right now

On Thu, Mar 22, 2012 at 10:56 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

On Thu, Mar 22, 2012 at 10:54 AM, Andrea Aime
<andrea.aime@anonymised.com> wrote:

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow&gt;
Let me know if that seems good to commit.

I'm buried right now, but I'll have a look as soon as I can (the weekend at
worst)

Sounds good, thanks.

Gabriel

Cheers
Andrea

--
-------------------------------------------------------
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

-------------------------------------------------------

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

5

Additionally, I think the following patch would be good, if my
understanding of how the module works is right.

The thing is that when there are more than one controller and there is
a timeout set, all but the first one can be called with a negative
maxWait argument, hence forcing the controller to add the request to
the queue. Instead, the callback should short circuit, because the
timeout has been exceeded already. The effect is more noticeable the
greater the load, since the actual elapsed time between calls to the
different controllers can vary more.

+++ b/src/extension/control-flow/src/main/java/org/geoserver/flow/ControlFlowCallback.java
@@ -73,7 +73,7 @@ public class ControlFlowCallback extends
AbstractDispatcherCallback implements
                 for (FlowController flowController : controllers) {
                     if(timeout > 0) {
                         long maxWait = maxTime - System.currentTimeMillis();
- if(!flowController.requestIncoming(request, maxWait))
+ if(maxWait <= 0 ||
!flowController.requestIncoming(request, maxWait))
                             throw new HttpErrorCodeException(503,
"Requested timeout out while waiting to be executed");
                      } else {
                         flowController.requestIncoming(request, -1);

On Thu, Mar 22, 2012 at 4:20 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

Btw, I see now that SingleIpFlowController should rather extend
SingleQueueFlowController, so take that for granted, just no time to
update the patch right now

On Thu, Mar 22, 2012 at 10:56 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

On Thu, Mar 22, 2012 at 10:54 AM, Andrea Aime
<andrea.aime@anonymised.com> wrote:

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow&gt;
Let me know if that seems good to commit.

I'm buried right now, but I'll have a look as soon as I can (the weekend at
worst)

Sounds good, thanks.

Gabriel

Cheers
Andrea

--
-------------------------------------------------------
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

-------------------------------------------------------

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

6

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com…> wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow>
Let me know if that seems good to commit.

https://github.com/groldan/geoserver/commit/8384f8255e6e17f729a2abb88a84f130c9d4e9ad#L1L-1
Purely aestetic, before and after are the same to me. +0

https://github.com/groldan/geoserver/commit/6c09e02c465d847f9567c90ac72ee8f5cac470aa
Nice fix, but I don’t understand the logic change in the test

https://github.com/groldan/geoserver/commit/f1d42a3aa1c1d4efd3d862cb1d80c89d1f3fa630
The fix seems correct, misses a test to prove it

https://github.com/groldan/geoserver/commit/503af1278786d0de4fef70720db771bfe992b42b
https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea
Lots of code reshuffling to get the same functionality. I don’t like this kind of change, since
it starts from a pretty simple and easy to understand set of code and ends up with
a version that has more code.
Maybe by looking at the diffs I missed some actual behavior fix.
Anyways, I’ll let Juan Marin review this one, since he introduced the classes.

https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea
Agree that the field should not be there, it’s not thread safe.
Mumble, a test would be good.
There is also a lot of other changes unrelated to the field removal… which look ok too,
they add hot reloading that was missing in the first version, as far as I can see

Cheers
Andrea

–

Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

7

On Thu, Mar 22, 2012 at 9:28 PM, Gabriel Roldan <groldan@anonymised.com…> wrote:

Additionally, I think the following patch would be good, if my
understanding of how the module works is right.

The thing is that when there are more than one controller and there is
a timeout set, all but the first one can be called with a negative
maxWait argument, hence forcing the controller to add the request to
the queue. Instead, the callback should short circuit, because the
timeout has been exceeded already. The effect is more noticeable the
greater the load, since the actual elapsed time between calls to the
different controllers can vary more.

+++ b/src/extension/control-flow/src/main/java/org/geoserver/flow/ControlFlowCallback.java
@@ -73,7 +73,7 @@ public class ControlFlowCallback extends
AbstractDispatcherCallback implements
for (FlowController flowController : controllers) {
if(timeout > 0) {
long maxWait = maxTime - System.currentTimeMillis();

  • if(!flowController.requestIncoming(request, maxWait))
  • if(maxWait <= 0 ||
    !flowController.requestIncoming(request, maxWait))
    throw new HttpErrorCodeException(503,
    “Requested timeout out while waiting to be executed”);
    } else {
    flowController.requestIncoming(request, -1);

Right, good catch

Cheers
Andrea

–

Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

8

Response inline

On Sun, Mar 25, 2012 at 4:41 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow>
Let me know if that seems good to commit.

https://github.com/groldan/geoserver/commit/8384f8255e6e17f729a2abb88a84f130c9d4e9ad#L1L-1
Purely aestetic, before and after are the same to me. +0

https://github.com/groldan/geoserver/commit/6c09e02c465d847f9567c90ac72ee8f5cac470aa
Nice fix, but I don’t understand the logic change in the test

https://github.com/groldan/geoserver/commit/f1d42a3aa1c1d4efd3d862cb1d80c89d1f3fa630
The fix seems correct, misses a test to prove it

https://github.com/groldan/geoserver/commit/503af1278786d0de4fef70720db771bfe992b42b

I’m OK with this change, unless I’m missing something it’s got more to do with the design of the classes than an actual bug (i.e. not extend but implement interface), it’s probably more flexible this way.

https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea

This one is more important I think. I missed a synchronized block when modifying the queues which will cause problems under concurrent access. And the ArrayList of IPs is not needed, actually it could potentially leak memory under heavy load.

Lots of code reshuffling to get the same functionality. I don’t like this kind of change, since
it starts from a pretty simple and easy to understand set of code and ends up with
a version that has more code.
Maybe by looking at the diffs I missed some actual behavior fix.
Anyways, I’ll let Juan Marin review this one, since he introduced the classes.

https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea
Agree that the field should not be there, it’s not thread safe.
Mumble, a test would be good.
There is also a lot of other changes unrelated to the field removal… which look ok too,
they add hot reloading that was missing in the first version, as far as I can see

Thanks for the review to both of you

Cheers

Andrea

–

Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

–
Juan Marín Otero

9

ok, sorry for the slow reply.

2012/3/25 Juan Marín Otero <juan.marin.otero@anonymised.com>:

Response inline

On Sun, Mar 25, 2012 at 4:41 AM, Andrea Aime <andrea.aime@anonymised.com>
wrote:

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com>
wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow&gt;
Let me know if that seems good to commit.

https://github.com/groldan/geoserver/commit/8384f8255e6e17f729a2abb88a84f130c9d4e9ad#L1L-1
Purely aestetic, before and after are the same to me. +0

yup, I'm -1 on it actually. That was supposed for the
SingleIpController to reuse the queue without necessarily extending
QueueController, and getting rid of this change is what I meant by
take for granted it should extend SingleQueueController, that I didn't
realize before.

https://github.com/groldan/geoserver/commit/6c09e02c465d847f9567c90ac72ee8f5cac470aa
Nice fix, but I don't understand the logic change in the test

Reason for the change in the test case is that isStale() is called
twice now every time it actually is stale. So now the test
configuration switches its state when it's actually called to load
the configuration.

For the rest, I'm with Juan this week so we're pair up and come up
with a cleaner patch.

Cheers,
Gabriel

https://github.com/groldan/geoserver/commit/f1d42a3aa1c1d4efd3d862cb1d80c89d1f3fa630
The fix seems correct, misses a test to prove it

https://github.com/groldan/geoserver/commit/503af1278786d0de4fef70720db771bfe992b42b

I'm OK with this change, unless I'm missing something it's got more to do
with the design of the classes than an actual bug (i.e. not extend but
implement interface), it's probably more flexible this way.

https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea

This one is more important I think. I missed a synchronized block when
modifying the queues which will cause problems under concurrent access. And
the ArrayList of IPs is not needed, actually it could potentially leak
memory under heavy load.

Lots of code reshuffling to get the same functionality. I don't like this
kind of change, since
it starts from a pretty simple and easy to understand set of code and ends
up with
a version that has more code.
Maybe by looking at the diffs I missed some actual behavior fix.
Anyways, I'll let Juan Marin review this one, since he introduced the
classes.

https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea
Agree that the field should not be there, it's not thread safe.
Mumble, a test would be good.
There is also a lot of other changes unrelated to the field removal...
which look ok too,
they add hot reloading that was missing in the first version, as far as I
can see

Thanks for the review to both of you

Cheers
Andrea

--
-------------------------------------------------------
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

-------------------------------------------------------

--
Juan Marín Otero

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

10

Okay, committed the fixes and improvements that were agreed on to both
trunk and 2.1.x (that is, did _not_ extract the queue class to its own
file).
Description and link to patches at http://jira.codehaus.org/browse/GEOS-4961

If anything else needs discussion I'll be glad to review/re-patch.

Cheers,
Gabriel

2012/3/26 Gabriel Roldan <groldan@anonymised.com>:

ok, sorry for the slow reply.

2012/3/25 Juan Marín Otero <juan.marin.otero@anonymised.com>:

Response inline

On Sun, Mar 25, 2012 at 4:41 AM, Andrea Aime <andrea.aime@anonymised.com>
wrote:

On Thu, Mar 22, 2012 at 2:50 PM, Gabriel Roldan <groldan@anonymised.com>
wrote:

Hey Andrea, please take a look at the following patches for the
control-flow module (last six of them):
<https://github.com/groldan/geoserver/commits/controlflow&gt;
Let me know if that seems good to commit.

https://github.com/groldan/geoserver/commit/8384f8255e6e17f729a2abb88a84f130c9d4e9ad#L1L-1
Purely aestetic, before and after are the same to me. +0

yup, I'm -1 on it actually. That was supposed for the
SingleIpController to reuse the queue without necessarily extending
QueueController, and getting rid of this change is what I meant by
take for granted it should extend SingleQueueController, that I didn't
realize before.

https://github.com/groldan/geoserver/commit/6c09e02c465d847f9567c90ac72ee8f5cac470aa
Nice fix, but I don't understand the logic change in the test

Reason for the change in the test case is that isStale() is called
twice now every time it actually is stale. So now the test
configuration switches its state when it's actually called to load
the configuration.

For the rest, I'm with Juan this week so we're pair up and come up
with a cleaner patch.

Cheers,
Gabriel

https://github.com/groldan/geoserver/commit/f1d42a3aa1c1d4efd3d862cb1d80c89d1f3fa630
The fix seems correct, misses a test to prove it

https://github.com/groldan/geoserver/commit/503af1278786d0de4fef70720db771bfe992b42b

I'm OK with this change, unless I'm missing something it's got more to do
with the design of the classes than an actual bug (i.e. not extend but
implement interface), it's probably more flexible this way.

https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea

This one is more important I think. I missed a synchronized block when
modifying the queues which will cause problems under concurrent access. And
the ArrayList of IPs is not needed, actually it could potentially leak
memory under heavy load.

Lots of code reshuffling to get the same functionality. I don't like this
kind of change, since
it starts from a pretty simple and easy to understand set of code and ends
up with
a version that has more code.
Maybe by looking at the diffs I missed some actual behavior fix.
Anyways, I'll let Juan Marin review this one, since he introduced the
classes.

https://github.com/groldan/geoserver/commit/5db0d229e012f2d1b7378e7e8cd85d47333da6ea
Agree that the field should not be there, it's not thread safe.
Mumble, a test would be good.
There is also a lot of other changes unrelated to the field removal...
which look ok too,
they add hot reloading that was missing in the first version, as far as I
can see

Thanks for the review to both of you

Cheers
Andrea

--
-------------------------------------------------------
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead

Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy

phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549

http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf

-------------------------------------------------------

--
Juan Marín Otero

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.