[GitHub] markusthoemmes commented on a change in pull request #2338: Prepare cache to change it by configuration
markusthoemmes commented on a change in pull request #2338: Prepare cache to
change it by configuration
URL:
https://github.com/apache/incubator-openwhisk/pull/2338#discussion_r131658957
##
File path:
common/scala/src/main/scala/whisk/core/database/CouchDbStoreProvider.scala
##
@@ -23,20 +23,25 @@ import whisk.common.Logging
import whisk.core.WhiskConfig
import whisk.spi.Dependencies
import whisk.spi.SpiFactory
+import whisk.core.entity.DocInfo
/**
* A CouchDB implementation of ArtifactStoreProvider
*/
class CouchDbStoreProvider extends ArtifactStoreProvider {
-def makeStore[D <: DocumentSerializer](config: WhiskConfig, name:
WhiskConfig => String)(
+def makeStore[D <: DocumentSerializer, CacheAbstraction](config:
WhiskConfig, name: WhiskConfig => String, cache:
Option[WhiskCache[CacheAbstraction, DocInfo]])(
implicit jsonFormat: RootJsonFormat[D],
actorSystem: ActorSystem,
-logging: Logging): ArtifactStore[D] = {
+logging: Logging): ArtifactStore[D, CacheAbstraction] = {
require(config != null && config.isValid, "config is undefined or not
valid")
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))
+new CouchDbRestStore[D, CacheAbstraction](config.dbProtocol,
config.dbHost, config.dbPort.toInt, config.dbUsername, config.dbPassword,
name(config), cache)
+}
+
+def makeCache[CacheAbstraction](): WhiskCache[CacheAbstraction, DocInfo] =
{
+new MultipleReadersSingleWriterCache[CacheAbstraction, DocInfo]()
Review comment:
Should we move this implementation to `ArtifactStoreProvider`? After all,
it's not relying on implementation details.
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 #2338: Prepare cache to change it by configuration
markusthoemmes commented on a change in pull request #2338: Prepare cache to
change it by configuration
URL:
https://github.com/apache/incubator-openwhisk/pull/2338#discussion_r129027934
##
File path:
common/scala/src/main/scala/whisk/core/database/MultipleReadersSingleWriterCache.scala
##
@@ -146,138 +142,127 @@ trait MultipleReadersSingleWriterCache[W, Winfo] {
override def toString = s"tid ${transid.meta.id}, state ${state.get}"
}
-/**
- * This method posts a delete to the backing store, and either directly
invalidates the cache entry
- * or informs any outstanding transaction that it must invalidate the
cache on completion.
- */
-protected def cacheInvalidate[R](key: Any, invalidator: => Future[R])(
+def cacheInvalidate[R](key: Any, invalidator: => Future[R])(
implicit ec: ExecutionContext, transid: TransactionId, logger:
Logging): Future[R] = {
-if (cacheEnabled) {
-logger.info(this, s"invalidating $key on delete")
-
-// try inserting our desired entry...
-val desiredEntry = Entry(transid, InvalidateInProgress, None)
-cache(key)(desiredEntry) flatMap { actualEntry =>
-// ... and see what we get back
-val currentState = actualEntry.state.get
-
-currentState match {
-case Cached =>
-// nobody owns the entry, forcefully grab ownership
-// note: if a new cache lookup is received while
-// the invalidator has not yet completed (and hence
the actual entry
-// removed from the cache), such lookup operations
will still be able
-// to return the value that is cached, and this is
acceptable (under
-// the eventual consistency model) as long as such
lookups do not
-// mutate the state of the cache to violate the
invalidation that is
-// about to occur (this is eventually consistent and
NOT sequentially
-// consistent since the cache lookup and the setting
of the
-// InvalidateInProgress bit are not atomic
+logger.info(this, s"invalidating $key on delete")
+
+// try inserting our desired entry...
+val desiredEntry = Entry(transid, InvalidateInProgress, None)
+cache(key)(desiredEntry) flatMap { actualEntry =>
+// ... and see what we get back
+val currentState = actualEntry.state.get
+
+currentState match {
+case Cached =>
+// nobody owns the entry, forcefully grab ownership
+// note: if a new cache lookup is received while
+// the invalidator has not yet completed (and hence the
actual entry
+// removed from the cache), such lookup operations will
still be able
+// to return the value that is cached, and this is
acceptable (under
+// the eventual consistency model) as long as such lookups
do not
+// mutate the state of the cache to violate the
invalidation that is
+// about to occur (this is eventually consistent and NOT
sequentially
+// consistent since the cache lookup and the setting of the
+// InvalidateInProgress bit are not atomic
+invalidateEntryAfter(invalidator, key, actualEntry)
+
+case ReadInProgress | WriteInProgress =>
+if (actualEntry.trySet(currentState, InvalidateWhenDone)) {
+// then the pre-existing owner will take care of the
invalidation
+invalidator
+} else {
+// the pre-existing reader or writer finished and so
must
+// explicitly invalidate here
invalidateEntryAfter(invalidator, key, actualEntry)
+}
-case ReadInProgress | WriteInProgress =>
-if (actualEntry.trySet(currentState,
InvalidateWhenDone)) {
-// then the pre-existing owner will take care of
the invalidation
-invalidator
-} else {
-// the pre-existing reader or writer finished and
so must
-// explicitly invalidate here
-invalidateEntryAfter(invalidator, key, actualEntry)
-}
-
-case InvalidateInProgress =>
-if (actualEntry == desiredEntry) {
-// we own the entry, so we are responsible for
cleaning it up
-invalidateEntryAfter(invalidator, key, actualEntry)
-
