[fixes #428] - Configurable configuration #462

emilzse · 2017-01-03T08:20:04Z

Created interface for configuration classes
Changed to reference the interface in projects (ConfigurationDispatcher)
Moved XMLConfiguration to own module
Implemented PostgresConfiguration class supporting WMS-layers
** See geowebcache-core-context.xml for configuration

* Changed to reference the interface in projects (ConfigurationDispatcher) * Moved XMLConfiguration to own module * Implemented PostgresConfiguration class

…to store on init

smithkm · 2017-02-15T22:23:28Z

Hi Emil, sorry that I've neglected your PRs for so long. I'll try to find time to review this before we hit feature freeze for the next release, but I don't have much hope about finding time to clear it. I am sorry as this does look impressive. It should probably have been brought up on the mailing list before you started work as well. I know your last thread kind of fizzled but this is fairly dramatic change and will have an impact on downstream projects like GeoServer. Those of us who have been with the project for a while tend to set a rather poor example about discussing changes before we start work I know but it is even more important for newcomers.

Again, sorry about leaving this so long and we do appreciate the work you're contributing.

smithkm · 2017-02-15T22:33:00Z

I just spoke to @tbarsballe who may be able to justify some time to review this, but unfortunately we're both rushing to get things done before the GT/GWC/GS code freeze ourselves so it's not too likely.

If you want to try to reduce the chance of problems during review. Make sure you have unit tests, have documentation, do whitespace correctly (4 spaces indent in code, 2 spaces in XML, do not use tabs), don't change existing whitespace unnecessarily, don't break GeoServer (or have the changes you need to make to GeoServer ready to go, ideally in a PR referencing this one) and if there's anything you think someone might have questions about, try to add a comment explaining it (in code or attached to the PR)

I can't guarantee that will get your PR done in time for the release, but it will help your chances.

emilzse · 2017-02-17T09:47:44Z

Hi, thanks for looking in to this. I understand that it can be hard to approve the new PostgresConfiguration class without proper time on your side to test it.

But for me the most important thing is that you approve and merge the change of moving the XMLConfiguration class out to a separate module and implement it from an interface. And then reference the interface from all other modules to be able have different implementations for it. Then it is just about configuring the needed implementation in spring configuration.

This way me and others could create their own implementations and if someone wants to use the PostgresConfiguration they can get it from the fork.

Best regards

smithkm · 2017-02-17T20:53:12Z

Actually the Postgres configuration class was less of a concern because it would be an optional thing to be plugged in. The concerns I have are with whether the changes made to allow it are going to break anything either in GWC or downstream, and how well those changes mesh with the design of GWC such as it is and the needs of the community.

If you can do a pull request with just the bare minimum API changes that need to get in I'll try to get it reviewed on Saturday.

To avoid the things that tend to delay PRs, make sure you don't have any non-semantic whitespace changes, update any relevant javadocs, add unit tests if appropriate, and describe your thinking when you make architectural decisions or do anything weird or "clever".

Also check that GeoServer still works with the changes. We do coordinated releases between GeoTools, GeoWebCache, and GeoServer so we can't break anything downstream at the last minute. If GeoServer needs to be updated, issue a PR there with those changes referencing the PR here on GWC with a note that it should not be merged until the GWC PR has been merged.

I've tossed you an invite to a Gitter chat channel so we might be able to resolve things more quickly. My time zone is UTC-8.

smithkm · 2017-02-18T00:10:02Z

One other thing, new Java files should have an LGPL 3.0 copyright header.

This project is a OSGeo "Community Project" and is supposed to be improving our code contribution story. It would be great if you visit http://www.osgeo.org/content/foundation/legal/licenses.html and fill in the a contribution license (sending it to secrectary@osgeo.org). This would save effort so that I do not have to double back and ask permission in the future. If we can't contact you at that time we may have to remove or re-implement your contributions. It's not presently a requirement for contribution to GWC though.

If you sign the agreement and send it to OSGeo, you can list them as the copyright holder in the header, otherwise put yourself or your employer as appropriate. You can see examples of the header on the other files. You could also contribute the code now, and then transfer the copyright later.

emilzse · 2017-02-22T07:33:27Z

If you can do a pull request with just the bare minimum API changes that need to get in I'll try to get it reviewed on Saturday.

I would say that i tried to make it as minimal as possible. I did redo this to avoid the formatting of not changed lines, removed the commenting you mentioned in the patch file i did attach before to the issue and so on.

…ostgres_configuration # Conflicts: # geowebcache/config_xml/src/main/java/org/geowebcache/config/XMLConfiguration.java

smithkm · 2017-03-24T01:23:17Z

Two things are that you haven't added any new unit tests to confirm your your new functionality works (I can see where you've updated some existing tests to avoid breaking them but that's it), and you haven't added any user documentation, Both of these are not only necessary in their own right but important tools for doing the review itself.

…ostgres_configuration # Conflicts: # geowebcache/config_xml/src/main/java/org/geowebcache/config/XMLConfiguration.java # geowebcache/core/src/main/java/org/geowebcache/layer/wms/WMSLayer.java

emilzse · 2017-04-05T08:44:40Z

I've tossed you an invite to a Gitter chat channel so we might be able to resolve things more quickly. My time zone is UTC-8.

I didnt see the invite. Have not used Gitter before so maybe missed something.

Two things are that you haven't added any new unit tests to confirm your your new functionality works

You want unit tests for the PostgresConfiguration class correct? Where and how do you want the documentation to be written?

smithkm

I'm a bit wary of using JSON for persistence when everywhere else we use XML for persistence of configuration. GWC is inconsistent enough as it is.

It's also a bit worrying that the Postgres configuration this only works for WMSLayers. I really don't want GWC to suffer from feature fragmentation where "Feature X only works if you don't use Feature Y". As it is I'm surprised we don't have more of that.

It looks like this should also affect GeoServer downstream. Please make a PR there referencing this one with any necessary changes to ensure GeoServer doesn't break. Making your new feature available isn't necessary. Be aware that

Please also add unit tests covering any new functionality or behaviour. The callbacks to the quota system and the Postgres configuration system in particular should be tested. Look at the Posgres disk quota tests for an example of how to do testing against a postgres instance. Our CI build server includes a Postgres instance that can run those tests.

Something this significant also needs some user documentation which is here https://github.com/GeoWebCache/geowebcache/tree/master/documentation/en/user

It's written in Restructured Text format and compiled into HTML using Sphinx.

Please also add copyright headers to any new Java files you made. If you're copying old code into a new class (like in AbsConfigurationDispatcher), please cite the origin so we can keep track of who contributed what.

smithkm · 2017-04-27T21:20:45Z

geowebcache/config_postgres/pom.xml

+		</dependency>
+
+		<dependency>
+			<groupId>com.google.code.gson</groupId>


We already have dependencies on json.org, jackson, and jettison, is yet another JSON library really necessary?

smithkm · 2017-04-27T21:23:31Z

geowebcache/config_postgres/pom.xml

+		</dependency>
+
+		<dependency>
+			<groupId>net.sf.ehcache</groupId>


We already have other in memory caches particularly Guava cache and Hazelcast used by the In Memory Blobstore. Is there a particular reason you added another new dependency to do this?

smithkm · 2017-04-27T21:32:32Z

geowebcache/config_postgres/src/main/java/org/geowebcache/config/JSONUtils.java

+    }
+
+    /**
+     * Takes an object and parses it into given class.


This is a bit vague. It's not really parsing the object so much as a string representation of it, a lexical cast/copy.

smithkm · 2017-04-27T21:47:14Z

geowebcache/config_postgres/src/main/java/org/geowebcache/config/PostgresConfiguration.java

+        }
+    }
+
+    public void setTemplate(String template) {


A lot of methods without Javadocs. Makes this difficult to follow.

smithkm · 2017-04-28T00:39:38Z

geowebcache/config_postgres/src/main/java/org/geowebcache/config/PostgresConfiguration.java

+                try {
+                    s = con.createStatement();
+                    rs = s.executeQuery(
+                            String.format(SELECT_EXISTS_BY_NAME, getSchema(), layerName));


Dumping values into SQL (Which you do in several places) without any sanitization seems like a bad idea. Admittedly this value shouldn't be possible to set for anyone who isn't an admin, but it still seems like a bad idea just on principle. If nothing else someone might use a reserved word as the name of their schema or include a space or some something silly like that.

smithkm · 2017-04-28T01:03:34Z

geowebcache/core/src/main/java/org/geowebcache/storage/CompositeBlobStore.java

-     *         {@link BlobStoreConfig#createInstance() created} of an enabled
-     *         {@link BlobStoreConfig}
+     * @param layers
+     *            used to get the layer's {@link TileLayer#getBlobStoreId() blobstore id}


Non-semantic whitespace again. Yes it does look nicer but it should be done separately.

smithkm · 2017-04-28T01:07:12Z

geowebcache/core/src/test/resources/org/geowebcache/config/geowebcache_1100.xml

@@ -1,212 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>


You seem to have simply deleted this file rather than moving it like you did for other versions.

smithkm · 2017-04-28T01:10:04Z

geowebcache/diskquota/core/src/main/java/org/geowebcache/diskquota/DiskQuotaMonitor.java

+         * tileLayer = tileLayerDispatcher.getTileLayer(layerName); } catch (GeoWebCacheException e)
+         * { e.printStackTrace(); continue; } cacheInfoBuilder.buildCacheInfo(tileLayer); } } <<<
+         */
+


Please don't leave commented out code unless it's an example. For an explanation of when something will happen/be used somewhere else in a way that may be confusing, just use a regular comment. To explain why you made a change, use a Pull Request comment.

smithkm · 2017-04-28T01:14:55Z

geowebcache/diskquota/core/src/main/java/org/geowebcache/diskquota/UsageStatsMonitor.java

+        // Iterable<TileLayer> allLayers = tileLayerDispatcher.getLayerList();
+        // for (TileLayer layer : allLayers) {
+        // layer.addLayerListener(usageStatsProducer);
+        // }


Again, please don't leave commented out code unless it's an example or something like that.

smithkm · 2017-04-28T01:15:57Z

geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStore.java

-                }
+                // for (String layerName : layerNames) {
+                // createLayerInternal(layerName);
+                // }


Again, comment explaining where this is being done elsewhere is good, commented out code is bad.

emilzse · 2017-05-03T06:53:38Z

Thank you for review, will try to look into the changes you requested

tbarsballe · 2018-11-27T16:53:32Z

@emilzse Have you had a chance to look at the feedback and update the PR?
If this doesn't see any changes soon, it will likely be closed as abandoned.

emilzse · 2018-11-28T07:04:29Z

@emilzse Have you had a chance to look at the feedback and update the PR?
If this doesn't see any changes soon, it will likely be closed as abandoned.

I think if someone is interested they can use the fork. With the changes in current GWC version regarding API and configuration this PR needs to be redone

smithkm · 2018-11-28T23:19:22Z

Thanks @emilzse, sorry this ended up clashing with the other configuration work. I'll close the PR.

emilzse added 2 commits January 3, 2017 09:14

* Created interface for configuration classes

5d0be0d

* Changed to reference the interface in projects (ConfigurationDispatcher) * Moved XMLConfiguration to own module * Implemented PostgresConfiguration class

Fixes issue with testing JDBC quota store since layers are not added …

bc30a45

…to store on init

emilzse added 2 commits March 8, 2017 08:24

Merge remote-tracking branch 'refs/remotes/GeoWebCache/master' into p…

9591646

…ostgres_configuration # Conflicts: # geowebcache/config_xml/src/main/java/org/geowebcache/config/XMLConfiguration.java

merge

20b51a8

emilzse added 3 commits April 5, 2017 09:13

Merge remote-tracking branch 'refs/remotes/GeoWebCache/master' into p…

0164731

…ostgres_configuration # Conflicts: # geowebcache/config_xml/src/main/java/org/geowebcache/config/XMLConfiguration.java # geowebcache/core/src/main/java/org/geowebcache/layer/wms/WMSLayer.java

merge

ddda0fd

Updated versions

bd54aba

smithkm requested changes Apr 28, 2017

View reviewed changes

smithkm closed this Nov 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fixes #428] - Configurable configuration #462

[fixes #428] - Configurable configuration #462

emilzse commented Jan 3, 2017

smithkm commented Feb 15, 2017

smithkm commented Feb 15, 2017

emilzse commented Feb 17, 2017

smithkm commented Feb 17, 2017

smithkm commented Feb 18, 2017

emilzse commented Feb 22, 2017

smithkm commented Mar 24, 2017

emilzse commented Apr 5, 2017

smithkm left a comment •

edited

smithkm Apr 27, 2017

smithkm Apr 27, 2017

smithkm Apr 27, 2017

smithkm Apr 27, 2017

smithkm Apr 28, 2017

smithkm Apr 28, 2017

smithkm Apr 28, 2017

smithkm Apr 28, 2017

smithkm Apr 28, 2017

smithkm Apr 28, 2017

emilzse commented May 3, 2017

tbarsballe commented Nov 27, 2018

emilzse commented Nov 28, 2018

smithkm commented Nov 28, 2018

[fixes #428] - Configurable configuration #462

[fixes #428] - Configurable configuration #462

Conversation

emilzse commented Jan 3, 2017

smithkm commented Feb 15, 2017

smithkm commented Feb 15, 2017

emilzse commented Feb 17, 2017

smithkm commented Feb 17, 2017

smithkm commented Feb 18, 2017

emilzse commented Feb 22, 2017

smithkm commented Mar 24, 2017

emilzse commented Apr 5, 2017

smithkm left a comment • edited

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

Choose a reason for hiding this comment

emilzse commented May 3, 2017

tbarsballe commented Nov 27, 2018

emilzse commented Nov 28, 2018

smithkm commented Nov 28, 2018

smithkm left a comment •

edited