-
Notifications
You must be signed in to change notification settings - Fork 102
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
ES upgrade to 2.4.1 #808
base: master
Are you sure you want to change the base?
ES upgrade to 2.4.1 #808
Conversation
ChandraAddala
commented
May 11, 2017
- Upgrading ES to 2.4.4 and lucene to 5.5.2 - Changes to mapping document: Change from index_analyzer to analyzer - Changing max limit from 100,000 to 10,000; Externalized this limit. - Adding ESIntegTestCase for integration tests - Changes to ingest ES client to ElasticIO and ElasticTokensIO - Remove references to shaded libraries of old ES
- Removed old test library, elasticsearch-test - Common logic for ES setup is placed in HttpESIntegrationBase
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.
some questions
@@ -122,6 +122,7 @@ | |||
<!-- do not require zookeeper for these tests. --> | |||
<argLine>-DZOOKEEPER_CLUSTER=NONE</argLine> | |||
<argLine>-Dlog4j.configuration=file://${basedir}/src/test/resources/log4j-test.properties ${jacoco.agent.it.argLine} -Xmx1024m -XX:MaxPermSize=256m</argLine> | |||
<argLine>-Duser.language=en</argLine> |
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.
What's wrong with French or Korean??? 😂
@@ -37,7 +37,7 @@ | |||
|
|||
private static final char[] STRING_SEEDS = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890_".toCharArray(); | |||
protected static final Random RAND = new Random(System.currentTimeMillis()); | |||
protected static final ConcurrentHashMap<Locator, String> locatorToUnitMap = new ConcurrentHashMap<Locator, String>(); | |||
public static final ConcurrentHashMap<Locator, String> locatorToUnitMap = new ConcurrentHashMap<Locator, String>(); |
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.
Just curious, why are these methods and the above variable all made public?
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 class is in blueflood-core. It was earlier extended in blueflood-http test classes. Now instead of extending they contain this class (has-a relationship). I have to make them public to be able to call them.
blueflood-elasticsearch/pom.xml
Outdated
@@ -33,8 +33,101 @@ | |||
|
|||
|
|||
<build> | |||
<testResources> | |||
<testResource> | |||
<directory>src/integration-test/java</directory> |
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.
Why is this directory specified as a testResource? If there are some files in that directory needed to be copied to classpath, then those files should be moved to src/integration-test/resources
directory.
<id>add-integration-test-sources</id> | ||
<phase>generate-test-sources</phase> | ||
<goals> | ||
<goal>add-test-source</goal> |
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.
Do we need to do this? I know we did this in blueflood-core
but I'm wondering if it's just left over cruft there. Adding test sources should be done within the failsafe-plugin or surefire-plugin. There is a
<testSourceDirectory>src/integration-test/java</testSourceDirectory>
configuration element. Does that not work?
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 is not available in the version we are using 2.19. It is available from 2.2 onwards.
public class ElasticIOIntegrationTest extends BaseElasticTest { | ||
|
||
protected ElasticIO elasticIO; | ||
protected ElasticTokensIO elasticTokensIO; |
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.
So is this test changing from testing the Token indexes to just general ElasticSearch test?
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.
testing token indexes is in its own class ElasticTokensIOIntegrationTest
* -Dtests.security.manager=false (https://github.com/elastic/elasticsearch/issues/16459) | ||
* | ||
*/ | ||
public class ElasticTokensIOIntegrationTest extends BaseElasticTest { |
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.
👍 for separating this
Configuration.getInstance().setProperty(CoreConfig.DISCOVERY_MODULES.name(), "com.rackspacecloud.blueflood.io.ElasticIO"); | ||
Configuration.getInstance().setProperty(CoreConfig.EVENTS_MODULES.name(), "com.rackspacecloud.blueflood.io.EventElasticSearchIO"); | ||
Configuration.getInstance().setProperty(CoreConfig.TOKEN_DISCOVERY_MODULES.name(), "com.rackspacecloud.blueflood.io.ElasticTokensIO"); | ||
Configuration.getInstance().setProperty(CoreConfig.ENABLE_TOKEN_SEARCH_IMPROVEMENTS.name(), "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.
Do we not load a blueflood config for these tests? Should we be putting these in the config 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.
Thats correct. We dont load a separate config file. I just used the way it was being done before. It mostly takes defaults for other properties except a few of them.
@ChandraAddala do these changes work with ElasticSearch 1.x ? Once we merge this PR, we will essentially move our tests to ElasticSearch 2.x, while our Prod is still 1.x. I'm wondering what should be the order of things? |
Also, coverage seems to decrease 20% with this PR. Can you look to see if that's just a bug in coverall? Or was there something big we missed testing? |
a01fe21
to
1b2649f
Compare