Uploaded image for project: 'Sling'
  1. Sling
  2. SLING-3223

Donation of a replication module for Sling

    Details

      Description

      Issue to track donation of a replication module for Sling.
      Discussion thread: http://markmail.org/thread/ic62k5pc34ppb5ko
      Vote on dev@sling thread: http://markmail.org/thread/scz5arlfs5rgowg2
      Result vote on dev@sling: http://markmail.org/thread/ix7s4fzvsdwmxrpk

      1. SLING-3223.patch
        337 kB
        Tommaso Teofili

        Issue Links

        There are no Sub-Tasks for this issue.

          Activity

          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          The SLING-3254 IP clearance is done, I have now committed this patch in http://svn.apache.org/r1547378 - thanks for your contribution!

          I've made minor changes in http://svn.apache.org/r1547381 and http://svn.apache.org/r1547382, suggest creating distinct issues for the other suggested changes.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - The SLING-3254 IP clearance is done, I have now committed this patch in http://svn.apache.org/r1547378 - thanks for your contribution! I've made minor changes in http://svn.apache.org/r1547381 and http://svn.apache.org/r1547382 , suggest creating distinct issues for the other suggested changes.
          Hide
          teofili Tommaso Teofili added a comment -

          the IP Clearance has been done (SLING-3254)

          Show
          teofili Tommaso Teofili added a comment - the IP Clearance has been done ( SLING-3254 )
          Hide
          teofili Tommaso Teofili added a comment -

          Thanks Bertrand for looking into it, I agree that needs to be fixed (also integration tests are needed IMO), I'll start working on that right now.

          Show
          teofili Tommaso Teofili added a comment - Thanks Bertrand for looking into it, I agree that needs to be fixed (also integration tests are needed IMO), I'll start working on that right now.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          I generated a test coverage report (by using our 19-SNAPSHOT and building with -P jacoco-report) and test coverage is fairly low, 29% globally and it looks like it's mostly happy cases that are tested.

          IMO a replication module deserves much better coverage, as troubleshooting it can be quite costly.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - I generated a test coverage report (by using our 19-SNAPSHOT and building with -P jacoco-report) and test coverage is fairly low, 29% globally and it looks like it's mostly happy cases that are tested. IMO a replication module deserves much better coverage, as troubleshooting it can be quite costly.
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          Here's a digest of the patch that we should use for the votes and IP clearance that's related to it:

          SHA1(SLING-3223.patch.txt)= ee628f4556c71c19fa09ae4e58fc7b32182da11b

          Show
          bdelacretaz Bertrand Delacretaz added a comment - Here's a digest of the patch that we should use for the votes and IP clearance that's related to it: SHA1( SLING-3223 .patch.txt)= ee628f4556c71c19fa09ae4e58fc7b32182da11b
          Hide
          teofili Tommaso Teofili added a comment -

          but then, is there really a difference between activate and deacticate ?

          the difference is just in the package is deserialized which is up to the ReplicationPackageBuilder (which takes care of both serialization and deserialization)

          ok. consider using a different name, then

          good suggestion

          I would get rid of it and put it into an utility class, or into the servlet where you need it. it just "pollutes" the set of adapters in sling.

          I see your point, I wouldn't put it into the servlet, perhaps a utility class works good.

          thanks Tobias.
          I there's no other concern I'd start the vote on dev@ next week.

          Show
          teofili Tommaso Teofili added a comment - but then, is there really a difference between activate and deacticate ? the difference is just in the package is deserialized which is up to the ReplicationPackageBuilder (which takes care of both serialization and deserialization) ok. consider using a different name, then good suggestion I would get rid of it and put it into an utility class, or into the servlet where you need it. it just "pollutes" the set of adapters in sling. I see your point, I wouldn't put it into the servlet, perhaps a utility class works good. thanks Tobias. I there's no other concern I'd start the vote on dev@ next week.
          Hide
          tripod Tobias Bocanegra added a comment -

          >> btw: how is deactivate implemented? not via an empty content package?
          > an empty package (see VoidReplicationPackage)
          but then, is there really a difference between activate and deacticate ?

          > they [the authentication handlers] are used for authenticate TransportHandlers which may deliver content to either an HTTP endpoint (most often) or not (e.g. file system, or other)
          ok. consider using a different name, then.

          >> the adapter from SlingRequest to ReplicationPackage is really questionable in IMO. It is used in a very specific case and the adaption is considerably expensive. Or do you foresee that other clients of this API adapt a request to a ReplicationPackage ?
          > I agree that can be much improved, that's just used from the receiving servlet for reading a replication package stream to install the package on the receiving instance.
          I would get rid of it and put it into an utility class, or into the servlet where you need it. it just "pollutes" the set of adapters in sling.

          Show
          tripod Tobias Bocanegra added a comment - >> btw: how is deactivate implemented? not via an empty content package? > an empty package (see VoidReplicationPackage) but then, is there really a difference between activate and deacticate ? > they [ the authentication handlers ] are used for authenticate TransportHandlers which may deliver content to either an HTTP endpoint (most often) or not (e.g. file system, or other) ok. consider using a different name, then. >> the adapter from SlingRequest to ReplicationPackage is really questionable in IMO. It is used in a very specific case and the adaption is considerably expensive. Or do you foresee that other clients of this API adapt a request to a ReplicationPackage ? > I agree that can be much improved, that's just used from the receiving servlet for reading a replication package stream to install the package on the receiving instance. I would get rid of it and put it into an utility class, or into the servlet where you need it. it just "pollutes" the set of adapters in sling.
          Hide
          teofili Tommaso Teofili added a comment -

          HTTP POST (/w parameters) to http://host:port/system/replication/agent/configuration/foo for updating configuration of agent foo

          typo there, sorry, it's :

          HTTP POST (/w parameters) to http://host:port/system/replication/agent/foo/configuration for updating configuration of agent foo
          

          Makes me think you're using your own configuration mechanism instead of using OSGi configs, as Toby also notes. Is there a reason for that? Otherwise, using OSGi configs should allow you to get rid of some code, and reuse the standard webconsole and other tools to manage configs.

          I'm using OSGi configs (configuration factory /w sling:OsgiConfig nodes) for creating configurations then for retrieving / updating configurations I use a resource provider which maps the HTTP request to the resource

          /system/replication/agent/foo/configuration
          

          first to the agent /system/replication/agent/foo then I use ConfigurationAdmin to get the OSGi config for that agent.
          Maybe (probably, at this point ) I'm not aware of simpler existing mechanism for retrieving / updating OSGi configurations so a pointer there would be good.

          But all this can be handled once your code is donated, IMO it doesn't prevent us from committing your code once the paperwork is ok.

          good

          Show
          teofili Tommaso Teofili added a comment - HTTP POST (/w parameters) to http://host:port/system/replication/agent/configuration/foo for updating configuration of agent foo typo there, sorry, it's : HTTP POST (/w parameters) to http: //host:port/system/replication/agent/foo/configuration for updating configuration of agent foo Makes me think you're using your own configuration mechanism instead of using OSGi configs, as Toby also notes. Is there a reason for that? Otherwise, using OSGi configs should allow you to get rid of some code, and reuse the standard webconsole and other tools to manage configs. I'm using OSGi configs (configuration factory /w sling:OsgiConfig nodes) for creating configurations then for retrieving / updating configurations I use a resource provider which maps the HTTP request to the resource /system/replication/agent/foo/configuration first to the agent /system/replication/agent/foo then I use ConfigurationAdmin to get the OSGi config for that agent. Maybe (probably, at this point ) I'm not aware of simpler existing mechanism for retrieving / updating OSGi configurations so a pointer there would be good. But all this can be handled once your code is donated, IMO it doesn't prevent us from committing your code once the paperwork is ok. good
          Hide
          bdelacretaz Bertrand Delacretaz added a comment -

          I haven't looked at your code yet but the following:

          HTTP GET http://host:port/system/replication/agent/configuration/foo for retrieving configuration of agent foo
          HTTP POST (/w parameters) to http://host:port/system/replication/agent/configuration/foo for updating configuration of agent foo
          

          Makes me think you're using your own configuration mechanism instead of using OSGi configs, as Toby also notes. Is there a reason for that? Otherwise, using OSGi configs should allow you to get rid of some code, and reuse the standard webconsole and other tools to manage configs.

          But all this can be handled once your code is donated, IMO it doesn't prevent us from committing your code once the paperwork is ok.

          Show
          bdelacretaz Bertrand Delacretaz added a comment - I haven't looked at your code yet but the following: HTTP GET http: //host:port/system/replication/agent/configuration/foo for retrieving configuration of agent foo HTTP POST (/w parameters) to http: //host:port/system/replication/agent/configuration/foo for updating configuration of agent foo Makes me think you're using your own configuration mechanism instead of using OSGi configs, as Toby also notes. Is there a reason for that? Otherwise, using OSGi configs should allow you to get rid of some code, and reuse the standard webconsole and other tools to manage configs. But all this can be handled once your code is donated, IMO it doesn't prevent us from committing your code once the paperwork is ok.
          Hide
          teofili Tommaso Teofili added a comment - - edited

          thanks for your comments Tobias.

          way to little documentation (javadoc, general architecture)

          yes, while I have some doc written in a README.md, javadoc needs to be improved.

          Don't use HTTP headers to transport additional information. this makes it harder to get request correctly though proxies and firewalls ... If you use customer HTTP headers, it's best practice to prepend an app specific prefix, eg: X-Sling-Replication-Action

          good points, then trying to get rid of the headers at all sounds like the best option

          why do you expose a ReplicationConfigurationServlet and not rely on default mechanisms to update config, e.g. update the config via SlingPostServlet or via felix console? (Is there some functionality missing in Sling or does every component needs to provide it's own management REST API?)

          it's not something missing, I'd like to provide an HTTP/REST API for working with agents, that including triggering replication, checking/updating configuration, checking queues (TBD), creating/deleting agent (TBD).
          If there's a smarter way of doing such things you may think of just let me know.
          Current HTTP API has:
          HTTP POST (/w parameters) to http://host:port/system/replication/agents for triggering replication of all existing agents
          HTTP POST (/w parameters) to http://host:port/system/replication/agent/foo for triggering replication of agent foo
          HTTP GET http://host:port/system/replication/agent/configuration/foo for retrieving configuration of agent foo
          HTTP POST (/w parameters) to http://host:port/system/replication/agent/configuration/foo for updating configuration of agent foo

          I would reconsider if Activate and Deactivate really are good names.

          sure, I agree "add" and "delete" sound more appropriate.

          btw: how is deactivate implemented? not via an empty content package?

          an empty package (see VoidReplicationPackage)

          I don't know if TriggerPathReplicationRule is really a sufficient filter. Usually it's hard to determined at what point a modified subtree is ready to replicate. you would probably need to add much more logic to it

          right, that's hard to do in general if e.g. create operations are not "atomic" (create /a/b, persist, create /a/b/c persist, create /a/b/c/d etc.) so depending on the use case something like collecting "bunch" of changes may work better; however that's good you raised the point as that will need to be handled properly.

          I don't quite understand AuthenticationHandler. Are those providers that are used to add authentication to the HTTP requests, or are they consumers that are used to authenticate replication payloads?

          it's the former, but it's not just for HTTP, they're used for authenticate TransportHandlers which may deliver content to either an HTTP endpoint (most often) or not (e.g. file system, or other)

          the adapter from SlingRequest to ReplicationPackage is really questionable in IMO. It is used in a very specific case and the adaption is considerably expensive. Or do you foresee that other clients of this API adapt a request to a ReplicationPackage ?

          I agree that can be much improved, that's just used from the receiving servlet for reading a replication package stream to install the package on the receiving instance.

          suggest to add package-info.java files with the bundle export information over <Export-Package> elements in the pom.xml

          agreed

          while collecting other feedback (if any) I'll work on the topics brought by Tobias.

          Show
          teofili Tommaso Teofili added a comment - - edited thanks for your comments Tobias. way to little documentation (javadoc, general architecture) yes, while I have some doc written in a README.md, javadoc needs to be improved. Don't use HTTP headers to transport additional information. this makes it harder to get request correctly though proxies and firewalls ... If you use customer HTTP headers, it's best practice to prepend an app specific prefix, eg: X-Sling-Replication-Action good points, then trying to get rid of the headers at all sounds like the best option why do you expose a ReplicationConfigurationServlet and not rely on default mechanisms to update config, e.g. update the config via SlingPostServlet or via felix console? (Is there some functionality missing in Sling or does every component needs to provide it's own management REST API?) it's not something missing, I'd like to provide an HTTP/REST API for working with agents, that including triggering replication, checking/updating configuration, checking queues (TBD), creating/deleting agent (TBD). If there's a smarter way of doing such things you may think of just let me know. Current HTTP API has: HTTP POST (/w parameters) to http://host:port/system/replication/agents for triggering replication of all existing agents HTTP POST (/w parameters) to http://host:port/system/replication/agent/foo for triggering replication of agent foo HTTP GET http://host:port/system/replication/agent/configuration/foo for retrieving configuration of agent foo HTTP POST (/w parameters) to http://host:port/system/replication/agent/configuration/foo for updating configuration of agent foo I would reconsider if Activate and Deactivate really are good names. sure, I agree "add" and "delete" sound more appropriate. btw: how is deactivate implemented? not via an empty content package? an empty package (see VoidReplicationPackage ) I don't know if TriggerPathReplicationRule is really a sufficient filter. Usually it's hard to determined at what point a modified subtree is ready to replicate. you would probably need to add much more logic to it right, that's hard to do in general if e.g. create operations are not "atomic" (create /a/b, persist, create /a/b/c persist, create /a/b/c/d etc.) so depending on the use case something like collecting "bunch" of changes may work better; however that's good you raised the point as that will need to be handled properly. I don't quite understand AuthenticationHandler. Are those providers that are used to add authentication to the HTTP requests, or are they consumers that are used to authenticate replication payloads? it's the former, but it's not just for HTTP, they're used for authenticate TransportHandlers which may deliver content to either an HTTP endpoint (most often) or not (e.g. file system, or other) the adapter from SlingRequest to ReplicationPackage is really questionable in IMO. It is used in a very specific case and the adaption is considerably expensive. Or do you foresee that other clients of this API adapt a request to a ReplicationPackage ? I agree that can be much improved, that's just used from the receiving servlet for reading a replication package stream to install the package on the receiving instance. suggest to add package-info.java files with the bundle export information over <Export-Package> elements in the pom.xml agreed while collecting other feedback (if any) I'll work on the topics brought by Tobias.
          Hide
          tripod Tobias Bocanegra added a comment -

          some quick comments:

          • way to little documentation (javadoc, general architecture)
          • Don't use HTTP headers to transport additional information. this makes it harder to get request correctly though proxies and firewalls
          • If you use customer HTTP headers, it's best practice to prepend an app specific prefix, eg: X-Sling-Replication-Action.
          • why do you expose a ReplicationConfigurationServlet and not rely on default mechanisms to update config, e.g. update the config via SlingPostServlet or via felix console? (Is there some functionality missing in Sling or does every component needs to provide it's own management REST API?)
          • ReplicationActionType names are probably coming from Adobe's replication - I would reconsider if Activate and Deactivate really are good names. btw: how is deactivate implemented? not via an empty content package?
            • alternative names for activate: "add", "put", "import", "post"
            • alternative names for deactivate: "delete"
          • I don't know if TriggerPathReplicationRule is really a sufficient filter. Usually it's hard to determined at what point a modified subtree is ready to replicate. you would probably need to add much more logic to it
          • I don't quite understand AuthenticationHandler. Are those providers that are used to add authentication to the HTTP requests, or are they consumers that are used to authenticate replication payloads?
            • If the first, I would rename them to something more specific, like: ReplicationCredentialsProvider
            • If the latter, I would really not reinvent the wheel here, and rely on authentication already present in Sling.
          • the adapter from SlingRequest to ReplicationPackage is really questionable in IMO. It is used in a very specific case and the adaption is considerably expensive. Or do you foresee that other clients of this API adapt a request to a ReplicationPackage ?
          • suggest to add package-info.java files with the bundle export information over <Export-Package> elements in the pom.xml
          Show
          tripod Tobias Bocanegra added a comment - some quick comments: way to little documentation (javadoc, general architecture) Don't use HTTP headers to transport additional information. this makes it harder to get request correctly though proxies and firewalls If you use customer HTTP headers, it's best practice to prepend an app specific prefix, eg: X-Sling-Replication-Action . why do you expose a ReplicationConfigurationServlet and not rely on default mechanisms to update config, e.g. update the config via SlingPostServlet or via felix console? (Is there some functionality missing in Sling or does every component needs to provide it's own management REST API?) ReplicationActionType names are probably coming from Adobe's replication - I would reconsider if Activate and Deactivate really are good names. btw: how is deactivate implemented? not via an empty content package? alternative names for activate: "add", "put", "import", "post" alternative names for deactivate: "delete" I don't know if TriggerPathReplicationRule is really a sufficient filter. Usually it's hard to determined at what point a modified subtree is ready to replicate. you would probably need to add much more logic to it I don't quite understand AuthenticationHandler . Are those providers that are used to add authentication to the HTTP requests, or are they consumers that are used to authenticate replication payloads? If the first, I would rename them to something more specific, like: ReplicationCredentialsProvider If the latter, I would really not reinvent the wheel here, and rely on authentication already present in Sling. the adapter from SlingRequest to ReplicationPackage is really questionable in IMO. It is used in a very specific case and the adaption is considerably expensive. Or do you foresee that other clients of this API adapt a request to a ReplicationPackage ? suggest to add package-info.java files with the bundle export information over <Export-Package> elements in the pom.xml
          Hide
          teofili Tommaso Teofili added a comment -

          here's the patch for the replication module.
          It's added under contrib/extensions/replication.
          I can post the sources for simpler review if needed.

          Show
          teofili Tommaso Teofili added a comment - here's the patch for the replication module. It's added under contrib/extensions/replication. I can post the sources for simpler review if needed.

            People

            • Assignee:
              bdelacretaz Bertrand Delacretaz
              Reporter:
              teofili Tommaso Teofili
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development