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.

Sign up for GitHub

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

Jump to bottom

Elasticsearch Version Resolve Capabilities #21

Merged

Symbianx merged 10 commits into nosinovacao:dev from Mozcatel:dev

Nov 6, 2017

Contributor

Mozcatel commented Oct 31, 2017 •

edited

Loading

Main features:

Added ElasticsearchVersionResolver;
Added a GetHttpWebRequest method on ConnectedVersionResolver that returns an HttpWebRequest object;
ServiceVersionResolver now uses the GetHttpWebRequest method on ConnectedVersionResolver
Added Elasticsearch type to name.dependencies.v1.jschema
Added unit and integration testing to the implementations above

Bruno Carvalho and others added 5 commits

October 26, 2017 14:51


          NAME-42 Elasticsearch resolver fixed and added integration test

5ec02c7


          NAME-42 cleaning and refactoring code:

9fc62eb


          NAME-42 major refactoring for elastic components

adfed39


          Merge branch 'dev' of https://github.com/nosinovacao/name-sdk into dev

0ef5ca4


          NAME-42 ElasticsearchVersionResolverTests fix

5b473ee

Symbianx reviewed

View reviewed changes

Contributor

Symbianx left a comment •

edited

Loading

Thanks for the pull request, I have reviewed the code.

The integration tests are failing, because you have a typo in the connection string.

Also, you should add both elasticsearch services to the docker_init.sh script, which waits for the services initialization.

Fix these things and I'll merge, good job 👍

integration-tests/NAME.IntegrationTests/Constants.cs Outdated

@@ @@ -17,6 +17,11 @@ public static class Constants { @@
                   public static string SpecificRabbitVersion;
                   public static string SpecificServiceVersion;
+                  // Elasticsearch variables
+                  public static string LatestElasticHostname;

Contributor

Symbianx Oct 31, 2017

Please use the full name of Elasticsearch, should be LatestElasticsearchHostname

integration-tests/NAME.IntegrationTests/Constants.cs Outdated

@@ @@ -17,6 +17,11 @@ public static class Constants { @@
                   public static string SpecificRabbitVersion;
                   public static string SpecificServiceVersion;
+                  // Elasticsearch variables
+                  public static string LatestElasticHostname;
+                  public static string SpecificElasticHostname;

Contributor

Symbianx Oct 31, 2017

Same as above

integration-tests/NAME.IntegrationTests/Constants.cs Outdated

+                  // Elasticsearch variables
+                  public static string LatestElasticHostname;
+                  public static string SpecificElasticHostname;
+                  public static string SpecificElasticVersion;

Contributor

Symbianx Oct 31, 2017

Same as above

integration-tests/NAME.IntegrationTests/Constants.cs Outdated

@@ @@ -31,5 +36,9 @@ public static class Constants { @@
                       SpecificMongoVersion = Environment.GetEnvironmentVariable("SPECIFIC_MONGO_VERSION") ?? "3.0.0";
                       SpecificRabbitVersion = Environment.GetEnvironmentVariable("SPECIFIC_RABBIT_VERSION") ?? "3.6.5";
                       SpecificServiceVersion = Environment.GetEnvironmentVariable("SPECIFIC_SERVICE_VERSION") ?? "1.0.0";
+                      LatestElasticHostname = Environment.GetEnvironmentVariable("LATEST_ELASTIC_HOSTNAME") ?? "localhost";

Contributor

Symbianx Oct 31, 2017

The Environment Variables should also contain the full name of Elasticsearch, should be LATEST_ELASTICSEARCH_HOSTNAME

integration-tests/NAME.IntegrationTests/Constants.cs Outdated

@@ @@ -31,5 +36,9 @@ public static class Constants { @@
                       SpecificMongoVersion = Environment.GetEnvironmentVariable("SPECIFIC_MONGO_VERSION") ?? "3.0.0";
                       SpecificRabbitVersion = Environment.GetEnvironmentVariable("SPECIFIC_RABBIT_VERSION") ?? "3.6.5";
                       SpecificServiceVersion = Environment.GetEnvironmentVariable("SPECIFIC_SERVICE_VERSION") ?? "1.0.0";
+                      LatestElasticHostname = Environment.GetEnvironmentVariable("LATEST_ELASTIC_HOSTNAME") ?? "localhost";
+                      SpecificElasticHostname = Environment.GetEnvironmentVariable("SPECIFIC_ELASTIC_HOSTNAME") ?? "localhost";

Contributor

Symbianx Oct 31, 2017

Same as above

integration-tests/NAME.IntegrationTests/Elasticsearch/ElasticsearchVersionResolverTests.cs Outdated

+                      [Trait("TestCategory", "Integration")]
+                      public async Task GetVersions_SpecificVersion()
+                      {
+                          ElasticsearchVersionResolver resolver = new ElasticsearchVersionResolver(new StaticConnectionStringProvider($"http://{ Constants.SpecificMongoHostname }:9200"), 10000, 10000);

Contributor

Symbianx Oct 31, 2017

Typo, should be SpecificElasticsearchHostname instead of SpecificMongoHostname

integration-tests/NAME.IntegrationTests/Elasticsearch/ElasticsearchVersionResolverTests.cs Outdated

+                      [Trait("TestCategory", "Integration")]
+                      public async Task GetVersions_LatestVersion()
+                      {
+                          ElasticsearchVersionResolver resolver = new ElasticsearchVersionResolver(new StaticConnectionStringProvider($"http://{ Constants.LatestMongoHostname }:9200"), 10000, 10000);

Contributor

Symbianx Oct 31, 2017

same as above

integration-tests/docker-compose.yml Outdated

@@ @@ -9,6 +9,8 @@ services: @@
                     - specific-mongo
                     - specific-rabbitmq
                     - dummy-service
+                    - elastic

Contributor

Symbianx Oct 31, 2017

Same, use the full name of elasticsearch, to keep things consistent.
The same for the rest of the file.

src/NAME/Elasticsearch/ElasticsearchVersionResolver.cs

+                          HttpWebResponse response;
+                          try
+                          {
+                              var request = this.GetHttpWebRequest(connectionString, SupportedDependencies.Elasticsearch.ToString());

Contributor

Symbianx Oct 31, 2017

Although you set the ContinueTimeout in the GetHttpWebRequest, it is not enough, because the Elasticsearch HTTP server may accept the connection and not send the response, so we need a read write timeout here, take a look at the ServiceVersionResolver for clues on how to do that.

src/NAME/Elasticsearch/ElasticsearchVersionResolver.cs Outdated

+                              var request = this.GetHttpWebRequest(connectionString, SupportedDependencies.Elasticsearch.ToString());
+                              response = await request.GetResponseAsync() as HttpWebResponse;
+                          }
+                          catch (Exception e)

Contributor

Symbianx Oct 31, 2017

You should catch the most relevant exception instead of the base Exception, in this case it is System.Net.WebException.

A39591 and others added 2 commits

October 31, 2017 17:35


          NAME-42 ReadTimeout added to Elasticsearch,and some fixes

ee82ad9


          NAME-42 stylecop fixes

e905ac3

Symbianx reviewed

View reviewed changes

src/NAME/Elasticsearch/ElasticsearchVersionResolver.cs

+                              {
+                                  using (response = await getResponseTask.ConfigureAwait(false) as HttpWebResponse)
+                                  {
+                                      using (var reader = new StreamReader(response.GetResponseStream()))

Contributor

Symbianx Oct 31, 2017

I think we should handle the StatusCode before reading the response stream.

Contributor Author

Mozcatel Oct 31, 2017

I don't view it as necessary in this case of Elasticsearch though, as the request is to the root of the server itself.. if it doesn't return you the expected value, it's because the server isn't up, thus being a server-side error resulting in a WebException.


          NAME-42 VersionParsingException redundancy fix

d89052a

Member

suvl commented Nov 2, 2017

@Mozcatel you're just missing the change on the json schema.

Please add it to the recently introduced schema file.

After that, I believe we're ready to merge.


          NAME-42 Elasticsearch type added to json schema

408a38f

Symbianx reviewed

View reviewed changes

name.dependencies.v1.jschema Outdated

@@ @@ -73,6 +73,7 @@ @@
                                               "RabbitMq",
                                               "MongoDb",
                                               "SqlServer"
+              								"Elasticsearch"

Contributor

Symbianx Nov 6, 2017

Please maintain consistency and use spaces instead of tabs. After that i will merge


          NAME-42 schema fix

d417f00

Symbianx merged commit a90a391 into nosinovacao:dev

This was referenced Nov 6, 2017

Support for getting Elasticsearch info #19

Closed

Support Redis version resolving #28

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment