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

Switch to Spring for the REST API #111

Closed
marcomicera opened this issue Nov 9, 2020 · 4 comments · Fixed by #123
Closed

Switch to Spring for the REST API #111

marcomicera opened this issue Nov 9, 2020 · 4 comments · Fixed by #123
Assignees
Labels
help wanted Extra attention is needed Priority: High After critical issues are fixed, these should be dealt with

Comments

@marcomicera
Copy link
Member

As far as the REST API is concerned, there are still a few issues:

All (or part of) these problems could be solved together by switching from RESTeasy to Spring (especially for Spring DI).

This change would also affect integration tests #89 and Dockerization #101.

@marcomicera marcomicera added help wanted Extra attention is needed Priority: High After critical issues are fixed, these should be dealt with labels Nov 9, 2020
@marcomicera marcomicera self-assigned this Nov 9, 2020
@gousiosg
Copy link
Contributor

gousiosg commented Nov 9, 2020

Could you please explain how / why the problems you describe will be solved with Spring?

@marcomicera
Copy link
Member Author

Could you please explain how / why the problems you describe will be solved with Spring?

Sure, sorry for my brevity.

  1. First things first: the Spring DI. Spring has some dedicated annotations like @Autowired and @Service that already allowed me to have a fully-functional CDI system. This simplifies unit testing a lot REST API unit tests #87.
  2. I already noticed that this Spring version of the REST API doesn't suffer from most of the timeouts we had before. I think this is due to Spring's embedded Tomcat Servlet container. I didn't manage to find out why Jetty caused timeouts, so a very dumb solution would be to just get rid of it.
  3. For concurrency, I think this is going to be handled by Spring & Tomcat. I didn't find any simple solution for Jetty rather than spawning threads manually Make sure the REST API handles concurrent requests #93 (comment). I think this could be achieved with a simple server.tomcat.max-threads=400 with the Spring's embedded Tomcat. There seem to be lots of other tweaks as well for Spring.

@marcomicera
Copy link
Member Author

@ilyagrishkov @MihhailSokolov do I have to keep Main.java and RestAPIPlugin.java that implements the PF4J Plugin interface and DBConnector?
It's a bit complicated to launch Spring through such classes.

I'd just use:

@SpringBootApplication
public class RestApplication {

    public static void main(String[] args) {
        SpringApplication.run(RestApplication.class, args);
    }
}

With a Spring @Component that connects to the KnowledgeBase at the beginning:

@Component
public class KnowledgeBaseConnector {

    @PostConstruct
    public void connectToKnowledgeBase() { /* ... */ }
}

So instead of launching it with mvn exec:java it would be: mvn spring-boot:run.
So technically, not a PF4J plugin anymore: is it ok?

@marcomicera
Copy link
Member Author

Done in #123.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed Priority: High After critical issues are fixed, these should be dealt with
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants