The Crawler now registers itself at the instance registry #24
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 13 commits
August 1, 2018 16:52
Initial functionality to verify the instance registry is available on startup
Changed infrastructure of Configuration to support the IR concept.
Still yields runtime exception, dependency 'spray-client' is no compatible with scala 2.12 . For scala 2.12 it it contents are part of akka-http-core, this needs to be upgraded.
Code Cleanup, API extensions
…h instance in the configuration.
…tdown / exception during preflight checks.
…gistry # Conflicts: # build.sbt # src/main/scala/de/upb/cs/swt/delphi/crawler/storage/ElasticReachablePreflightCheck.scala
ghost
assigned
Codecov Report
@@ Coverage Diff @@
## develop #24 +/- ##
==========================================
- Coverage 8.22% 7.33% -0.89%
==========================================
Files 36 38 +2
Lines 888 995 +107
Branches 12 23 +11
==========================================
Hits 73 73
- Misses 815 922 +107
Continue to review full report at Codecov.
|
bhermann
requested changes
Sep 7, 2018
|
|
||
| lazy val elasticsearchClientUri: ElasticsearchClientUri = ElasticsearchClientUri({ | ||
| if(elasticsearchInstance.portnumber.isEmpty){ | ||
| elasticsearchInstance.iP.getOrElse("elasticsearch://localhost:" + defaultElasticSearchPort) |
There was a problem hiding this comment.
Why is localhost hard coded here?
At first glance this looks a bit strange.
| if(elasticsearchInstance.portnumber.isEmpty){ | ||
| elasticsearchInstance.iP.getOrElse("elasticsearch://localhost:" + defaultElasticSearchPort) | ||
| }else{ | ||
| elasticsearchInstance.iP.getOrElse("elasticsearch://localhost") + ":" + elasticsearchInstance.portnumber.getOrElse(defaultElasticSearchPort) |
| case Success(instance) => instance | ||
| case Failure(_) => Instance( | ||
| None, | ||
| Some(sys.env.getOrElse("DELPHI_ELASTIC_URI","elasticsearch://localhost")), |
There was a problem hiding this comment.
In the original code there was a port given. Does it work without giving it the default port?
src/main/scala/de/upb/cs/swt/delphi/crawler/io/swagger/client/model/Instance.scala
Show resolved
Hide resolved
| Success(configuration) | ||
| } | ||
| } recover { case e => | ||
| if(configuration.usingInstanceRegistry) InstanceRegistry.sendMatchingResult(isElasticSearchReachable = false, configuration) |
There was a problem hiding this comment.
Having the condition at all places where it is used can be dangerous.
Could we always make the call and decide this in the method? (or through inheritance)
src/main/scala/de/upb/cs/swt/delphi/crawler/instancemanagement/InstanceRegistry.scala
Outdated
Show resolved
Hide resolved
build.sbt
Outdated
| @@ -70,6 +73,7 @@ libraryDependencies ++= Seq( | |||
| ) | |||
|
|
|||
| resolvers += "Sonatype OSS Snapshots" at "https://oss.sonatype.org/content/repositories/snapshots" | |||
| resolvers ++= Seq(Resolver.mavenLocal) | |||
There was a problem hiding this comment.
Why do we need this local resolver?
added 8 commits
September 8, 2018 12:00
bhermann
approved these changes
Sep 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The configuration file has been extended to now contain an attribute "InstanceRegistryUri". The crawler tries to connect to this URI on startup. If successful, it registers itself at the Instance Registry, retrieves an ElasticSearch instance from there and uses it instead of the default one specified in the config, and deregisters on error / shutdown.
If the crawler cannot connect to the Instance Registry, it will print a warning and continue the execution by using the default ElasticSearch instance specified in the config.
This addresses issue #21 .
Note: The loading of the configuration file "Hermes.conf" in HermesAnalyzer.scala line 101 using the ClassLoader failed for me both on Linux and Windows. The Hermes implementation reported an error that the configuration file "cannot be found or read".