Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple the crawler service for use in other frameworks #740

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

BenDol
Copy link
Contributor

@BenDol BenDol commented Oct 6, 2015

This is following a Spring implementation soon, though I don't know how you guys want the package breakdown for a Spring implementation since the crawler service assumes Guice.

Todo:

  • Decouple existing service servlet and filter.
  • Write Spring implementation for filter and service.
  • Test the new Guice implementation.
  • Update documentation with new configurations.

Questions:

  • Should the gwtp-crawler-service be renamed to gwtp-crawler-appengine?

Package Changes:

gwtp-core
1

gwtp-crawler
2

Sample Configurations:

Crawl Filter

@Configuration
public class CrawlFilterModule extends AbstractCrawlFilterModule {

    @Bean
    @Override
    protected String serviceUrl() {
        return "http://crawlservice.appspot.com/";
    }

    @Bean
    @Override
    protected String crawlKey() {
        return "123456";
    }

    // Can also override optional Logger
}

Crawler Service

@Configuration
public class CrawlerModule extends AbstractCrawlerServiceModule {

    @Bean
    @Override
    protected String crawlKey() {
        return "123456";
    }

    // Can also override optional WebClient, CrawlCacheService, or Logger
}

In relation to issue #739

This is following a Spring implementation soon, though I don't know how you guys want the package breakdown for a Spring implementation since the crawler service assumes Guice.
Currently the Spring implementation is untested (as well as the Guice changes).

If someone else that is more experienced using Guice could see if there will be any issues that would be great!
Separated the classes to 'filter' and 'service' sub packages.
@olafleur
Copy link
Member

olafleur commented Nov 2, 2015

Hi @BenDol : do you need some code review or help with that pull request?

@BenDol
Copy link
Contributor Author

BenDol commented Nov 13, 2015

Its functional, just the guice implementation needs to be tested. This is t allow me to use Spring backend for the crawler service.

@@ -14,12 +14,13 @@
* the License.
*/

package com.gwtplatform.crawler.server;
package com.gwtplatform.crawler.server.guice;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a breaking change. We should either use inheritance or duplicate it and support classes from both the new and old packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really matter if its a breaking change in this case? Since its a simple import change?

Copy link
Contributor Author

@BenDol BenDol Jun 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I just added a @Deprecated and directed them to the new ServiceKey annotation? Rather than bloat the code with unnecessary legacy support.

@Chris-V
Copy link
Member

Chris-V commented Jan 4, 2016

The changes look good so far. Please make sure the Guice implementation still works fine (I didn't see anything that would break it, but still).

Also be careful about package changes, those are breaking changes and we should support both the old and new packages if we rename them.


gwtp-crawler-service be renamed to gwtp-crawler-appengine?

The name change makes sense, though we'll have to keep a deprecated gwtp-crawler-service that inherits gwtp-crawler-appengine at first.

@olafleur
Copy link
Member

@BenDol what can we do to help you with that PR?

@BenDol
Copy link
Contributor Author

BenDol commented Jun 5, 2016

It would be a great help if someone could test this with a Guice implementation. I don't use Guice and don't have the time at the moment to figure it out and test it.

Also I am currently resolving the mentioned breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants