[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r131842341
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -36,10 +38,17 @@ class CouchDbStoreProvider extends ArtifactStoreProvider {
require(config.dbProvider == "Cloudant" || config.dbProvider ==
"CouchDB", "Unsupported db.provider: " + config.dbProvider)
assume(Set(config.dbProtocol, config.dbHost, config.dbPort,
config.dbUsername, config.dbPassword, name(config)).forall(_.nonEmpty), "At
least one expected property is missing")
-new CouchDbRestStore[D](config.dbProtocol, config.dbHost,
config.dbPort.toInt, config.dbUsername, config.dbPassword, name(config))
+val storeName = name(config)
+stores.get(storeName) match {
Review comment:
There's `getOrElseUpdate` on scala's maps. You can use it like this:
```scala
storeName.getOrElseUpdate(storeName, new
CouchDbRestStore[D](config.dbProtocol, config.dbHost, config.dbPort.toInt,
config.dbUsername, config.dbPassword, storeName))
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r131842596
##
File path:
core/controller/src/main/scala/whisk/core/loadBalancer/LoadBalancerService.scala
##
@@ -77,16 +82,32 @@ trait LoadBalancer {
*/
def publish(action: ExecutableWhiskAction, msg:
ActivationMessage)(implicit transid: TransactionId):
Future[Future[Either[ActivationId, WhiskActivation]]]
+/**
+ * Return a message indicating the health of the containers and/or
container pool in general
+ * @return a Future[String] representing the heal response that will be
sent to the client
+ */
+def getHealthResponse: Future[String]
Review comment:
This should not be a `String` but rather a `JsObject` . It needs to contain
the health of each invoker specifically.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r132404422
##
File path:
core/controller/src/main/scala/whisk/core/loadBalancer/LoadBalancerService.scala
##
@@ -31,17 +31,19 @@ import akka.actor.ActorSystem
import akka.actor.Props
import akka.pattern.ask
import akka.util.Timeout
+import spray.json.DefaultJsonProtocol._
+import spray.json._
import whisk.common.Logging
import whisk.common.LoggingMarkers
import whisk.common.TransactionId
import whisk.core.WhiskConfig
import whisk.core.WhiskConfig._
import whisk.core.connector.MessagingProvider
-import whisk.core.connector.{ ActivationMessage, CompletionMessage }
+import whisk.core.connector.{ActivationMessage, CompletionMessage}
Review comment:
Can you try to adjust this formatting?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer URL: https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r132404790 ## File path: common/scala/src/main/scala/whisk/core/database/CouchDbRestStore.scala ## @@ -52,7 +52,8 @@ class CouchDbRestStore[DocumentAbstraction <: DocumentSerializer]( dbPort: Int, dbUsername: String, dbPassword: String, -dbName: String)(implicit system: ActorSystem, val logging: Logging, jsonFormat: RootJsonFormat[DocumentAbstraction]) +dbName: String, +cleanup:(String)=>Unit)(implicit system: ActorSystem, val logging: Logging, jsonFormat: RootJsonFormat[DocumentAbstraction]) Review comment: Why do you need to pass the `dbName` in? When you open the client you automatically have it at hand. `shutdown: => Unit` should suffice (note the whitespace please) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r132405059
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -18,16 +18,18 @@
package whisk.core.database
import akka.actor.ActorSystem
+import scala.collection.mutable
import spray.json.RootJsonFormat
import whisk.common.Logging
import whisk.core.WhiskConfig
import whisk.spi.Dependencies
-import whisk.spi.SpiFactory
+import whisk.spi.SingletonSpiFactory
/**
* A CouchDB implementation of ArtifactStoreProvider
*/
class CouchDbStoreProvider extends ArtifactStoreProvider {
+protected[database] val stores = mutable.Map [String,
CouchDbRestStore[_]]()
Review comment:
Whitespace:
```scala
protected[database] val stores = mutable.Map[String, CouchDbRestStore[_]]()
```
Could this be private?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r132405059
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -18,16 +18,18 @@
package whisk.core.database
import akka.actor.ActorSystem
+import scala.collection.mutable
import spray.json.RootJsonFormat
import whisk.common.Logging
import whisk.core.WhiskConfig
import whisk.spi.Dependencies
-import whisk.spi.SpiFactory
+import whisk.spi.SingletonSpiFactory
/**
* A CouchDB implementation of ArtifactStoreProvider
*/
class CouchDbStoreProvider extends ArtifactStoreProvider {
+protected[database] val stores = mutable.Map [String,
CouchDbRestStore[_]]()
Review comment:
Whitespace:
```scala
protected[database] val stores = mutable.Map[String, CouchDbRestStore[_]]()
```
Could this be private?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r132406052
##
File path:
core/controller/src/main/scala/whisk/core/loadBalancer/LoadBalancerService.scala
##
@@ -275,6 +296,14 @@ class LoadBalancerService(
private def generateHash(namespace: EntityName, action:
ExecutableWhiskAction): Int = {
(namespace.asString.hashCode() ^
action.fullyQualifiedName(false).asString.hashCode()).abs
}
+
+/** Returns a Map of invoker instance -> invoker state */
+override def getHealthResponse(): Future[JsObject] = {
+val res = allInvokers.map(_.map {
+case (instance, state) => s"invoker${instance.toInt}" ->
state.asString
+}.toMap.toJson.asJsObject)
+res
Review comment:
There is no need to store the intermediary result:
```scala
override def getHealthResponse(): Future[JsObject] = allInvokers.map(_.map {
case (instance, state) => s"invoker${instance.toInt}" ->
state.asString
}.toMap.toJson.asJsObject)
```
Also please refrain from using java style getter names like
`getHealthResponse`. I suggest `LoadBalancer.healthStatus()`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r132406663
##
File path:
core/controller/src/main/scala/whisk/core/loadBalancer/LoadBalancerService.scala
##
@@ -77,16 +82,32 @@ trait LoadBalancer {
*/
def publish(action: ExecutableWhiskAction, msg:
ActivationMessage)(implicit transid: TransactionId):
Future[Future[Either[ActivationId, WhiskActivation]]]
+/**
+ * Return a message indicating the health of the containers and/or
container pool in general
+ * @return a Future[String] representing the heal response that will be
sent to the client
+ */
+def getHealthResponse: Future[JsObject]
+}
+
+class LoadBalancerServiceProvider extends LoadBalancerProvider {
+override def getLoadBalancer(config: WhiskConfig, instance: InstanceId)
+(implicit logging: Logging, actorSystem: ActorSystem):
LoadBalancer = new LoadBalancerService(config, instance)
+}
+
+object LoadBalancerServiceProvider extends SpiFactory[LoadBalancerProvider]{
+override def apply(dependencies: Dependencies): LoadBalancerProvider = new
LoadBalancerServiceProvider
}
class LoadBalancerService(
config: WhiskConfig,
-instance: InstanceId,
-entityStore: EntityStore)(
+instance: InstanceId)(
implicit val actorSystem: ActorSystem,
logging: Logging)
extends LoadBalancer {
+/** Used to manage an action for testing invoker health */
+val entityStore = WhiskEntityStore.datastore(config)
Review comment:
What's the rationale for generating a new one (and deduplicate via the SPI)
vs. passing it through the `Dependencies` object? (Not necessarily arguing
against this, just curiosity)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r133315135
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -36,10 +38,14 @@ class CouchDbStoreProvider extends ArtifactStoreProvider {
require(config.dbProvider == "Cloudant" || config.dbProvider ==
"CouchDB", "Unsupported db.provider: " + config.dbProvider)
assume(Set(config.dbProtocol, config.dbHost, config.dbPort,
config.dbUsername, config.dbPassword, name(config)).forall(_.nonEmpty), "At
least one expected property is missing")
-new CouchDbRestStore[D](config.dbProtocol, config.dbHost,
config.dbPort.toInt, config.dbUsername, config.dbPassword, name(config))
+val storeName = name(config)
+stores.getOrElseUpdate(storeName, new
CouchDbRestStore[D](config.dbProtocol, config.dbHost, config.dbPort.toInt,
config.dbUsername, config.dbPassword, storeName,
+()=>stores.remove(storeName)))
+.asInstanceOf[CouchDbRestStore[D]]
Review comment:
Formatting this in a bit more context would make sense imho:
```scala
stores.getOrElseUpdate(storeName,
new CouchDbRestStore[D](config.dbProtocol, config.dbHost,
config.dbPort.toInt, config.dbUsername, config.dbPassword, storeName, () =>
stores.remove(storeName))
).asInstanceOf[CouchDbRestStore[D]]
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r133315204
##
File path:
tests/src/test/scala/whisk/core/controller/test/ControllerTestCommon.scala
##
@@ -17,18 +17,17 @@
package whisk.core.controller.test
-import scala.concurrent.{ Await, Future }
+import scala.concurrent.{Await, Future}
import scala.concurrent.ExecutionContext
-import scala.concurrent.duration.{ DurationInt, FiniteDuration }
+import scala.concurrent.duration.{DurationInt, FiniteDuration}
Review comment:
Please try to remove all of those whitespace changes
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r137176968
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -18,16 +18,18 @@
package whisk.core.database
import akka.actor.ActorSystem
+import scala.collection.mutable
import spray.json.RootJsonFormat
import whisk.common.Logging
import whisk.core.WhiskConfig
import whisk.spi.Dependencies
-import whisk.spi.SpiFactory
+import whisk.spi.SingletonSpiFactory
/**
* A CouchDB implementation of ArtifactStoreProvider
*/
class CouchDbStoreProvider extends ArtifactStoreProvider {
+private val stores = mutable.Map [String, CouchDbRestStore[_]]()
Review comment:
This should be a `TrieMap` to make it threadsafe.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r137177998
##
File path:
tests/src/test/scala/whisk/core/database/test/CouchDBStoreProviderTests.scala
##
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package whisk.core.database.test
+
+import common.StreamLogging
+import common.WskActorSystem
+import org.junit.runner.RunWith
+import org.scalatest.FlatSpec
+import org.scalatest.Matchers
+import org.scalatest.junit.JUnitRunner
+import whisk.core.WhiskConfig
+import whisk.core.WhiskConfig._
+import whisk.core.database.ArtifactStoreProvider
+import whisk.core.entity.WhiskActivation
+import whisk.spi.SpiLoader
+
+@RunWith(classOf[JUnitRunner])
+class CouchDBStoreProviderTests extends FlatSpec
+with Matchers
+with WskActorSystem
+with StreamLogging {
Review comment:
`StreamLogging` shouldn't be needed here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
markusthoemmes commented on a change in pull request #2584: SPI for Loadbalancer
URL:
https://github.com/apache/incubator-openwhisk/pull/2584#discussion_r137178270
##
File path:
tests/src/test/scala/whisk/core/database/test/CouchDBStoreProviderTests.scala
##
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package whisk.core.database.test
+
+import common.StreamLogging
+import common.WskActorSystem
+import org.junit.runner.RunWith
+import org.scalatest.FlatSpec
+import org.scalatest.Matchers
+import org.scalatest.junit.JUnitRunner
+import whisk.core.WhiskConfig
+import whisk.core.WhiskConfig._
+import whisk.core.database.ArtifactStoreProvider
+import whisk.core.entity.WhiskActivation
+import whisk.spi.SpiLoader
+
+@RunWith(classOf[JUnitRunner])
+class CouchDBStoreProviderTests extends FlatSpec
+with Matchers
+with WskActorSystem
+with StreamLogging {
+val config = new WhiskConfig(Map(
+dbProvider -> "CouchDB",
+dbProtocol -> "http",
+dbUsername -> "fake",
+dbPassword -> "fake",
+dbHost -> "fake",
+dbPort -> "1234",
+dbActivations -> "activations_fake"))
+
+
+val artifactStoreProvider = SpiLoader.get[ArtifactStoreProvider]()
+val store1FirstLoad =
artifactStoreProvider.makeStore[WhiskActivation](config, _.dbActivations)
+val store1SecondLoad =
artifactStoreProvider.makeStore[WhiskActivation](config, _.dbActivations)
+
+behavior of "CouchDBStoreProvider"
+override def afterAll() {
+println("Shutting down store connections")
+store1FirstLoad.shutdown()
+//do not need to shutdown the second loaded store, but we'll do it
anyways since clients may do that also
+store1SecondLoad.shutdown()
+super.afterAll()
+}
+it should "load the same store from config for a specific artifact type"
in {
+store1FirstLoad shouldBe store1SecondLoad
+}
Review comment:
Given the more general implementation you had in another PR (as discussed in
Slack), should this be tested in a more general way? Also: Should we add a test
which asserts that two stores with different names are different instances?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
