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
JAMES-3266 Offer an option to disable ElasticSearch in Distributed James product #3425
Conversation
test this please |
.../guice/cassandra-guice/src/main/java/org/apache/james/CassandraJamesServerConfiguration.java
Outdated
Show resolved
Hide resolved
server/container/guice/cassandra-guice/src/main/java/org/apache/james/SearchModuleChooser.java
Show resolved
Hide resolved
...andra-rabbitmq-guice/src/main/java/org/apache/james/CassandraRabbitMQJamesConfiguration.java
Outdated
Show resolved
Hide resolved
|
We are working on documentation and every feature should be documented. Some questions and hints, take them or leave them:
|
This is meaningful, I'll handle these documentation topics. |
Unrelated test this please |
1 similar comment
Unrelated test this please |
[76fd1a1] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.22.2:test (default-test) on project james-server-mailets-integration-testing: There was a timeout or other error in the fork -> [Help 1] test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR deserves some more love before integrating it.
server/container/guice/cassandra-guice/src/main/java/org/apache/james/SearchModuleChooser.java
Outdated
Show resolved
Hide resolved
import com.github.fge.lambdas.Throwing; | ||
|
||
public class CassandraJamesServerConfiguration implements Configuration { | ||
public static class Builder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like a copy/paste of existing code. Why do we have to do that? I don't care about duplicating methods but there's some logic too that could be delegated to another object, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I make a simple refactor here, please take a look
nvduc91@1d3078f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do have a different proposal #3531
@@ -167,7 +160,7 @@ | |||
); | |||
|
|||
public static void main(String[] args) throws Exception { | |||
Configuration configuration = Configuration.builder() | |||
CassandraJamesServerConfiguration configuration = CassandraJamesServerConfiguration.builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How CassandraJamesServerConfiguration
is different from Configuration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need an explanation here.
Are you asking for...
Configuration configuration = CassandraJamesServerConfiguration.builder()
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I asked a question that is quite simple: we create a class B that is very similar to what we did with class A so I ask what is the difference between A and B.
You ask me if I want A a = B.builder()....build()
but I have no idea, how does it relate to me initial question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It differs from the module choices offered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so we may choose a naming pattern (and maybe some javadoc) that allow to discover which Configuration type one need. Could it be an inner class of the server we want to instantiate? I think that would make it more discoverable.
} | ||
|
||
static SearchConfiguration from(Configuration configuration) { | ||
if (configuration.getBoolean("enabled", true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration looks really bad to me: to have scanning search we have to go to elasticsearch configuration and disable it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please propose an alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like that #3425 (comment) ? Did you read my comments before asking ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont understand why i must disabled ES to get scanning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either you get scanning, either ES. Scanning is not backed at all by ES.
switch (searchConfiguration.getImplementation()) { | ||
case ElasticSearch: | ||
return ImmutableList.of( | ||
new ElasticSearchClientModule(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we have a single module for each implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can create a meta module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we agree on that or not?
...ntainer/guice/cassandra-guice/src/test/java/org/apache/james/WithCassandraBlobStoreTest.java
Outdated
Show resolved
Hide resolved
import org.junit.jupiter.api.extension.ExtendWith; | ||
|
||
@ExtendWith(WithScanningSearchExtension.class) | ||
class WithScanningSearchTest implements JmapJamesServerContract, MailsShouldBeWellReceived, JamesServerContract { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how much time it adds to the testsuite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much. Containers are already started, tables populated. Around a minute.
|
Co-authored-by: Raphaël Ouazana <rouazana@linagora.com>
…roperties configuration file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
test this please |
strange, master is broken or merge issue? test this please |
Possibly a merge issue I think. Let me check |
=> #3565 |
test this please |
Merged |
No description provided.