search

mapfish / mapfish-print

A component of MapFish for printing templated cartographic maps. This module is the Java serverside module.
http://mapfish.github.io/mapfish-print-doc/
BSD 2-Clause "Simplified" License
185 stars 417 forks source link

Different .yaml Configuration Broken (again) #128

Open geekdenz opened 10 years ago

geekdenz commented 10 years ago

I've just discovered with great frustration that the functionality that I added last year is broken again. Maybe we need to write a test that enforces that it's not.

The functionality is that one can have different .yaml files for different applications like we have. It is quite simple but somehow someone must've thought it is not important and got rid of it (again). This happened in the past already at least once.

jesseeichar commented 10 years ago

We have found several major memory leaks associated it. We have been trying to fix it but perhaps broke your functionality while doing it.

I am working on mapfish V3 and the app support is very well tested in that version. I will publish some of that work soon. It is a fairly drastic change though so I am waiting until I have something solid and is more backwards compatible than it currently is.

geekdenz commented 10 years ago

Hi @jesseeichar ,

Thanks! Great to hear you are working on V3!

Leaks are no good. But is it really the case? It would only be "leaking" the memory associated with the configuration objects. But one would want those to remain in memory, no?

Would be great if we had an ETA for that to fit in with our deadlines and whether we can decide to use it in our products.

jesseeichar commented 10 years ago

The leak was actually when you don't use the app parameter. It was reloading the configuration (and associated thread pool) on every request. Since this is currently the majority case I had to create a fix for... Since there are no tests well I apparently missed a usecase. Although that said I did test with requests that use the app parameter and it still worked so I thought it was correctly done.

Regarding ETA of V3. We have to put a version into production end of june. But that version isn't backwards compatible. We want to create some migration tools from V2 -> V3 and then we will release. Ideally by FOSS4G

geekdenz commented 10 years ago

Oh, I thought I catered for that in my code. I had noticed there were some changes done after that, so maybe it was a regression. Shows how important unit/functional tests are!

End of June is not far away! I could help with the app parameter issue if you think I can. If you are more into it at the moment, that's cool.

So, are your V3 commits up in this repo or do you have it private or somewhere else at the moment?

jesseeichar commented 10 years ago

At the moment V3 is in a private repository. I don't want people to freak out before we have the final design and migration path decided.

V3 is drastically different because it uses JasperSoft for doing the report templating and styling and mapfish focuses on the map rendering and the web API.

I am doing 2 days of work on the project next week so I will try to fix this then.

jesseeichar commented 10 years ago

I have been trying to reproduce the error and it is working for me. I put app:'<pathToConfig.yaml>' in the request json and it finds the configuration file and loads it correctly. I used the absolute path I admit but it worked perfectly.

geekdenz commented 10 years ago

So, you are using 

{ "app": "someyaml.yaml" }

?

We're using

{ "app": "someyaml" }

and then append the '.yaml' in the print module. The first way may be considered more correct, but it would be useful to know if it indeed is the way it was re-implemented. It's not a biggy, we can change the application logic which would fix it for us.

jesseeichar commented 10 years ago

I think a pushed a fix yesterday.

On Tue, May 27, 2014 at 4:07 AM, Tim-Hinnerk Heuer <notifications@github.com

wrote:

So, you are using

{ "app": "someyaml.yaml" }

?

We're using

{ "app": "someyaml" }

and then append the '.yaml' in the print module. The first way may be considered more correct, but it would be useful to know if it indeed is the way it was re-implemented. It's not a biggy, we can change the application logic which would fix it for us.

— Reply to this email directly or view it on GitHubhttps://github.com/mapfish/mapfish-print/issues/128#issuecomment-44229478 .

geekdenz commented 10 years ago

Thanks @jesseeichar .

I will be on my honey moon next week, so can only check it out when I'm back the following week. Hopefully I'll get around to writing some tests for this somehow if you haven't done so already or if you think it's not necessary with the new version in line.

jesseeichar commented 10 years ago

Congratulations have fun on your honeymoon

— Sent from my phone

On Fri, May 30, 2014 at 12:47 PM, Tim-Hinnerk Heuer notifications@github.com wrote:

Thanks @jesseeichar .

I will be on my honey moon next week, so can only check it out when I'm back the following week. Hopefully I'll get around to writing some tests for this somehow if you haven't done so already or if you think it's not necessary with the new version in line.

Reply to this email directly or view it on GitHub: https://github.com/mapfish/mapfish-print/issues/128#issuecomment-44638152