github / elasticsearch-srv-discovery

Elasticsearch discovery with SRV records
MIT License
34 stars 33 forks source link

Add Support to ES 2.X #25

Open falencastro opened 8 years ago

falencastro commented 8 years ago

It's compiling fine with maven (-Dmaven.test.skip=true).

Issues:

Details:

chrismwendt commented 8 years ago

Wonderful! Thank you, @falencastro ✨ I'm taking a look at it now 😃

falencastro commented 8 years ago

Hanging issue solved. I was running with a default java.policy file, just needed to replace it with: grant { permission java.security.AllPermission; }; Everything is working fine now! :smiley:

chrismwendt commented 8 years ago

Elasticsearch itself recently switched from Maven to Gradle (which is why I had done so for this plugin as well). Why did you roll back to Maven?

chrismwendt commented 8 years ago

This PR mixes tabs and spaces. Can you make it all 4-space indentation like it was before?

chrismwendt commented 8 years ago

I believe that you can actually remove src/main/java/org/elasticsearch/discovery/srv/SrvDiscoveryModule.java. I did so in the other WIP: #22

chrismwendt commented 8 years ago
  • Tests fail due to java.lang.IllegalArgumentException: Unknown Discovery type [srvtest].

I managed to solve this with https://github.com/github/elasticsearch-srv-discovery/pull/22/files#diff-c7aaa6daa650a88b917f442c2745dcabR59

It's pretty hacky because it pollutes the discovery namespace with something that should only be visible during testing, but I couldn't figure out another way around it. Let me know if you are able to get the test to work.

falencastro commented 8 years ago

Hi,

I implemented your suggestions. From your last PR:

Still happening. Once you create this directory manually, test goes ahead but gets stuck at 85% after starting elasticsearch.

In ES 5 theres a gradle plugin called esplugin, which will solve this, something like this will generate a new plugin-descriptor.properties:

esplugin {
  name 'srv-discovery'
  version '2.3.3'
  description 'SRV record discovery plugin'
  classname 'org.elasticsearch.plugin.discovery.srv.SrvDiscoveryPlugin'
}

Fixed on build.gradle:

jar {
    manifest {
        attributes("Class-Path": configurations.compile.collect { "lib/$it.name" }.join(' '))
    }
}
chrismwendt commented 8 years ago

Were you able to at least build the tests? gradle build seems to hang on me:

> gradle build
:compileJava UP-TO-DATE
:processResources UP-TO-DATE
:classes UP-TO-DATE
:jar UP-TO-DATE
:distTar UP-TO-DATE
:distZip UP-TO-DATE
:assemble UP-TO-DATE
:compileTestJava UP-TO-DATE
:processTestResources UP-TO-DATE
:testClasses UP-TO-DATE
> Building 76% > :test^C

When I run the test in IntelliJ, it also hangs at what appears to be the end:

NOTE: All tests run in this JVM: [SrvDiscoveryIntegrationTest]

I'm not sure what's going on there.

  • Tests fail due to java.nio.file.NoSuchFileException: .../build/resources/test

Still happening. Once you create this directory manually, test goes ahead but gets stuck at 85% after starting elasticsearch

Any idea why it's getting stuck during the test run?

Thanks for looking into the versioning - I think we can tolerate hard-coding it until ES 5.

And thanks for fixing that TextParseException - I wouldn't have thought of that fix.

mfussenegger commented 8 years ago

If I may chime in:

It's pretty hacky because it pollutes the discovery namespace with something that should only be visible during testing, but I couldn't figure out another way around it. Let me know if you are able to get the test to work.

I think you could use a stub implementation like we did in https://github.com/crate/crate/blob/master/dns-discovery/src/test/java/io/crate/discovery/SrvDiscoveryIntegrationTest.java#L45

I think that way the whole srvtest code could be removed.

Tests fail due to java.nio.file.NoSuchFileException: .../build/resources/test

I think this could be solved by adding

sourceSets {
    test {
        output.resourcesDir = null
    }
}

to the build.gradle

Tests hang for me as well at 85%

Didn't investigate but things I've noticed:

The package namespace isn't correct.

In SrvDiscoveryPlugin it is package org.elasticsearch.plugin.discovery.srv but should be package org.elasticsearch.plugin.discovery.

I think the tests need to define the plugin in order to load it:

    @Override
    protected Collection<Class<? extends Plugin>> nodePlugins() {
        return pluginList(SrvDiscoveryPlugin.class);
    }

And in SrvDiscoveryPlugin in onModule it should probably check the discovery.type setting and only load the components if it matches srv.