You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2020/11/09 18:45:52 UTC
[GitHub] [openwhisk] bdoyle0182 opened a new pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
bdoyle0182 opened a new pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024
## Description
This allows the operator to specifically set the instanceId for a unique name in zookeeper from the command line when starting the invoker. Added a comment that this command option should not be used unless you are sure that the invokerId does not exist for any unique name
For context, we use hostname for our invoker unique names. We have hosts that no longer exist and therefore invokers are created backfilled in the controller invoker pool for those invokers that will never be started again. This causes the invoker hashing algorithm to be off since these invokers are always considered down. This change allows us to easily move our highest lexicographical invokers into those slots in zookeeper without manually going into zookeeper to do so.
## Related issue and scope
- [ ] I opened an issue to propose and discuss this change (#????)
## My changes affect the following components
- [ ] API
- [ ] Controller
- [ ] Message Bus (e.g., Kafka)
- [ ] Loadbalancer
- [x] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [ ] Tests
- [ ] Deployment
- [ ] CLI
- [ ] General tooling
- [ ] Documentation
## Types of changes
- [ ] Bug fix (generally a non-breaking change which closes an issue).
- [x] Enhancement or new feature (adds new functionality).
- [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
## Checklist:
- [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- [x] I reviewed the [style guides](https://github.com/apache/openwhisk/wiki/Contributing:-Git-guidelines#code-readiness) and followed the recommendations (Travis CI will check :).
- [x] I added tests to cover my changes.
- [ ] My changes require further changes to the documentation.
- [ ] I updated the documentation where necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r521015205
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
I just pushed a change which checks all uniqueNames to see if the instance id already exists before overwriting it to this invoker. This isn't atomic like the counter is for assigning id's so it's not perfect, but I think it's more than good enough for something that should be used for corrective actions to an out of sync fleet.
Next up, I would like to figure out a way to improve the dynamic id assigner so that it can handle these gaps and backfill if things are missing in an atomic way to this check of what ids are there. But that should be saved for another pr. This is good enough for now for us to correct things.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r520391764
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
If I got this correctly, I think there would be still data for obsolete unique names.
Shouldn't we remove them?
For example, let's assume data looks like this:
```
/invokers/idAssignment/mapping/host1 0 // obsolete
/invokers/idAssignment/mapping/host2 1 // obsolete
/invokers/idAssignment/mapping/host3 2
/invokers/idAssignment/mapping/host4 3
```
You can update this like,
```
/invokers/idAssignment/mapping/host1 0 // obsolete
/invokers/idAssignment/mapping/host2 1 // obsolete
/invokers/idAssignment/mapping/host3 0
/invokers/idAssignment/mapping/host4 1
```
But there will still be data for `host1` and `host2`.
If such names are used again by any chance, isn't it possible that id conflicts can happen?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r520253993
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.get
Review comment:
I know the existence of this value is already checked, but still, I hope we can rather use the asynchronous function all the time.
```suggestion
val newId = overwriteId.getOrElse("")
```
The default case would not happen, but if it happens, it would cause an exception when casting to `BigInt` and stop the invoker from being booted up.
So that we can figure out the reason for the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r522294381
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -22,50 +22,76 @@ import org.apache.curator.framework.recipes.shared.SharedCount
import org.apache.curator.retry.RetryUntilElapsed
import org.apache.openwhisk.common.Logging
+import scala.collection.JavaConverters._
+
/**
* Computes the instanceId for invoker
*
* @param connectionString zooKeeper connection string
*/
private[invoker] class InstanceIdAssigner(connectionString: String)(implicit logger: Logging) {
- def getId(name: String): Int = {
+ def setAndGetId(name: String, overwriteId: Option[Int] = None): Int = {
logger.info(this, s"invokerReg: creating zkClient to $connectionString")
val retryPolicy = new RetryUntilElapsed(5000, 500) // retry at 500ms intervals until 5 seconds have elapsed
val zkClient = CuratorFrameworkFactory.newClient(connectionString, retryPolicy)
zkClient.start()
zkClient.blockUntilConnected()
logger.info(this, "invokerReg: connected to zookeeper")
- val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val rootPath = "/invokers/idAssignment/mapping"
+ val myIdPath = rootPath + s"/$name"
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
Review comment:
It appears that way yea, but I'd rather deal with that in a separate PR since it's already existing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r521015205
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
I just pushed a change which checks all uniqueNames to see if the instance id already exists before overwriting it to this invoker. This isn't atomic like the counter is for assigning id's so it's not perfect, but I think it's more than good enough for something that should be used for corrective actions to an out of sync fleet.
Next up, I would like to figure out a way to improve the dynamic id assigner so that it can handle these gaps and backfill if things are missing in an atomic way to this check of what ids are there. But that should be saved for another pr. This is good enough for now for us to correct things.
exception case should be covered in a unit test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r522434586
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
Done, it should delete the mapping with the instance id before it overwrites
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 merged pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 merged pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r520299983
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.get
Review comment:
sounds good
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r522295689
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
Yea that's right the data would have to be gone in zookeeper before the overwrite attempt happens. I guess I could try to delete the existing mapping once I find it and then overwrite the id
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#issuecomment-726353524
I made a couple changes to make this better and assigning ids more infallible:
1. As noted in the discussion above, if the invoker id existing for another name; it will delete that mapping before reassigning to the new name.
2. You can't overwrite with an invoker id that doesn't fit into the size of the invoker pool. i.e. if the invoker pool is size 2, I can't overwrite with id 2. only 0 or 1.
3. When dynamically assigning a new id, it now does so based on the number of invoker nodes in the zookeeper list rather than the atomic count. That way when reassigning things, it will always stay in sync with the true size of invokers if assigning a new id dynamically. This should maintain atomicity because it still uses the atomic counter as a lock and will re get the invokers size if it doesn't acquire the lock and would then include the new invoker that had the lock at the time. This is important to do because obviously it better tracks the true size of your invoker fleet and is important for allowing overwriting. For example, we want to move invokers 25-30 to invokers 0-5. When we do that, 25-30 would no longer exist; but our counter is still at 31 so if we add a new node it will be 31 leaving 25-30 forever empty. With this change, after moving 25-30 to 0-5 it will recognize there's actually only 25 invokers so when we add a new node it will correctly use 25 for the new node
.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#issuecomment-727085970
Tested this out in one of our regions. Works great
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r521204189
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
Not quite sure this would fix your problem.
It seems the path is not transient so that even if an obsolete invoker no longer exists, there will be the path and data in zookeeper.
So you would be unable to overwrite the existing ID all the time.
Is that correct?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r520416442
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
Yes that's correct. I added a comment that you should not use this overwrite unless you are sure that there is not another invoker that shares the id.
I would like to make it cleaner to update invoker host mapping, but currently it's tracked by a single atomic counter in zookeeper which makes this complicated since it only goes up. And since the invokers are hostname -> id mappings, you would need to get all invokers and look at all of the ids to determine what is the lowest id available to add it as (which I'm not sure if you can do atomically like with the counter I'm not super familiar with zookeeper). This is just meant to be a manual operation used for corrections when your fleet is out of sync. Do you have any better ideas because I do think it's a problem you can never really go back and remap invoker hosts to a different instance id
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r520421284
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
I feel like it's a bit picky.
How about removing the obsolete data from the zookeeper when overwriting the ID?
Maybe we can iterate the mappings and figure out the invoker with the overwritten ID.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] rabbah commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r521458110
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -22,50 +22,76 @@ import org.apache.curator.framework.recipes.shared.SharedCount
import org.apache.curator.retry.RetryUntilElapsed
import org.apache.openwhisk.common.Logging
+import scala.collection.JavaConverters._
+
/**
* Computes the instanceId for invoker
*
* @param connectionString zooKeeper connection string
*/
private[invoker] class InstanceIdAssigner(connectionString: String)(implicit logger: Logging) {
- def getId(name: String): Int = {
+ def setAndGetId(name: String, overwriteId: Option[Int] = None): Int = {
logger.info(this, s"invokerReg: creating zkClient to $connectionString")
val retryPolicy = new RetryUntilElapsed(5000, 500) // retry at 500ms intervals until 5 seconds have elapsed
val zkClient = CuratorFrameworkFactory.newClient(connectionString, retryPolicy)
zkClient.start()
zkClient.blockUntilConnected()
logger.info(this, "invokerReg: connected to zookeeper")
- val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val rootPath = "/invokers/idAssignment/mapping"
+ val myIdPath = rootPath + s"/$name"
+ val assignedId = if (overwriteId.isEmpty) {
Review comment:
scalaism here - rather than check if the option is empty, you can do
```scala
overwriteId.map(id => the not empty case ).getOrElse( the empty case )
```
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -22,50 +22,76 @@ import org.apache.curator.framework.recipes.shared.SharedCount
import org.apache.curator.retry.RetryUntilElapsed
import org.apache.openwhisk.common.Logging
+import scala.collection.JavaConverters._
+
/**
* Computes the instanceId for invoker
*
* @param connectionString zooKeeper connection string
*/
private[invoker] class InstanceIdAssigner(connectionString: String)(implicit logger: Logging) {
- def getId(name: String): Int = {
+ def setAndGetId(name: String, overwriteId: Option[Int] = None): Int = {
logger.info(this, s"invokerReg: creating zkClient to $connectionString")
val retryPolicy = new RetryUntilElapsed(5000, 500) // retry at 500ms intervals until 5 seconds have elapsed
val zkClient = CuratorFrameworkFactory.newClient(connectionString, retryPolicy)
zkClient.start()
zkClient.blockUntilConnected()
logger.info(this, "invokerReg: connected to zookeeper")
- val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val rootPath = "/invokers/idAssignment/mapping"
+ val myIdPath = rootPath + s"/$name"
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
+
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.get
+
+ //check if the invokerId already exists for another unique name
+ val instanceIdExists = zkClient
+ .getChildren()
+ .forPath(rootPath)
+ .asScala
+ .map(uniqueName => {
+ val idPath = rootPath + s"/$uniqueName"
Review comment:
```suggestion
val idPath = s"$rootPath/$uniqueName"
```
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -22,50 +22,76 @@ import org.apache.curator.framework.recipes.shared.SharedCount
import org.apache.curator.retry.RetryUntilElapsed
import org.apache.openwhisk.common.Logging
+import scala.collection.JavaConverters._
+
/**
* Computes the instanceId for invoker
*
* @param connectionString zooKeeper connection string
*/
private[invoker] class InstanceIdAssigner(connectionString: String)(implicit logger: Logging) {
- def getId(name: String): Int = {
+ def setAndGetId(name: String, overwriteId: Option[Int] = None): Int = {
logger.info(this, s"invokerReg: creating zkClient to $connectionString")
val retryPolicy = new RetryUntilElapsed(5000, 500) // retry at 500ms intervals until 5 seconds have elapsed
val zkClient = CuratorFrameworkFactory.newClient(connectionString, retryPolicy)
zkClient.start()
zkClient.blockUntilConnected()
logger.info(this, "invokerReg: connected to zookeeper")
- val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val rootPath = "/invokers/idAssignment/mapping"
+ val myIdPath = rootPath + s"/$name"
Review comment:
```suggestion
val myIdPath = s"$rootPath/$name"
```
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -22,50 +22,76 @@ import org.apache.curator.framework.recipes.shared.SharedCount
import org.apache.curator.retry.RetryUntilElapsed
import org.apache.openwhisk.common.Logging
+import scala.collection.JavaConverters._
+
/**
* Computes the instanceId for invoker
*
* @param connectionString zooKeeper connection string
*/
private[invoker] class InstanceIdAssigner(connectionString: String)(implicit logger: Logging) {
- def getId(name: String): Int = {
+ def setAndGetId(name: String, overwriteId: Option[Int] = None): Int = {
logger.info(this, s"invokerReg: creating zkClient to $connectionString")
val retryPolicy = new RetryUntilElapsed(5000, 500) // retry at 500ms intervals until 5 seconds have elapsed
val zkClient = CuratorFrameworkFactory.newClient(connectionString, retryPolicy)
zkClient.start()
zkClient.blockUntilConnected()
logger.info(this, "invokerReg: connected to zookeeper")
- val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val rootPath = "/invokers/idAssignment/mapping"
+ val myIdPath = rootPath + s"/$name"
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
Review comment:
it's "possible" this can recurse forever, should this give up after N trials?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r520421284
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
I feel like it's a bit picky.
How about removing the obsolete data from the zookeeper when overwriting the ID?
Maybe we can iterate the mappings and figure out the invoker with the overwritten ID.
I am also fine with the proper comment as this feature would not be used without any manual intervention.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r523009550
##########
File path: tests/src/test/scala/org/apache/openwhisk/core/invoker/test/InstanceIdAssignerTests.scala
##########
@@ -40,14 +40,33 @@ class InstanceIdAssignerTests extends FlatSpec with Matchers with StreamLogging
it should "assign fresh id" in {
val assigner = new InstanceIdAssigner(zkServer.getConnectString)
- assigner.getId("foo") shouldBe 0
+ assigner.setAndGetId("foo") shouldBe 0
}
it should "reuse id if exists" in {
val assigner = new InstanceIdAssigner(zkServer.getConnectString)
- assigner.getId("foo") shouldBe 0
- assigner.getId("bar") shouldBe 1
- assigner.getId("bar") shouldBe 1
+ assigner.setAndGetId("foo") shouldBe 0
+ assigner.setAndGetId("bar") shouldBe 1
+ assigner.setAndGetId("bar") shouldBe 1
}
+ it should "attempt to overwrite id for unique name if overwrite set" in {
+ val assigner = new InstanceIdAssigner(zkServer.getConnectString)
+ assigner.setAndGetId("foo") shouldBe 0
+ assigner.setAndGetId("bar", Some(0))
Review comment:
Should this be
```suggestion
assigner.setAndGetId("bar", Some(0)) shouldBe 0
```
?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a change in pull request #5024: Reset / Overwrite invokerId for unique name in zookeeper manually
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #5024:
URL: https://github.com/apache/openwhisk/pull/5024#discussion_r520421284
##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InstanceIdAssigner.scala
##########
@@ -38,34 +38,41 @@ private[invoker] class InstanceIdAssigner(connectionString: String)(implicit log
logger.info(this, "invokerReg: connected to zookeeper")
val myIdPath = "/invokers/idAssignment/mapping/" + name
- val assignedId = Option(zkClient.checkExists().forPath(myIdPath)) match {
- case None =>
- // path doesn't exist -> no previous mapping for this invoker
- logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
- val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
- idCounter.start()
+ val assignedId = if (overwriteId.isEmpty) {
+ Option(zkClient.checkExists().forPath(myIdPath)) match {
+ case None =>
+ // path doesn't exist -> no previous mapping for this invoker
+ logger.info(this, s"invokerReg: no prior assignment of id for invoker $name")
+ val idCounter = new SharedCount(zkClient, "/invokers/idAssignment/counter", 0)
+ idCounter.start()
- def assignId(): Int = {
- val current = idCounter.getVersionedValue()
- if (idCounter.trySetCount(current, current.getValue() + 1)) {
- current.getValue()
- } else {
- assignId()
+ def assignId(): Int = {
+ val current = idCounter.getVersionedValue()
+ if (idCounter.trySetCount(current, current.getValue() + 1)) {
+ current.getValue()
+ } else {
+ assignId()
+ }
}
- }
- val newId = assignId()
- idCounter.close()
- zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
- logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
- newId
+ val newId = assignId()
+ idCounter.close()
+ zkClient.create().creatingParentContainersIfNeeded().forPath(myIdPath, BigInt(newId).toByteArray)
+ logger.info(this, s"invokerReg: invoker $name was assigned invokerId $newId")
+ newId
- case Some(_) =>
- // path already exists -> there is a previous mapping for this invoker we should use
- val rawOldId = zkClient.getData().forPath(myIdPath)
- val oldId = BigInt(rawOldId).intValue
- logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
- oldId
+ case Some(_) =>
+ // path already exists -> there is a previous mapping for this invoker we should use
+ val rawOldId = zkClient.getData().forPath(myIdPath)
+ val oldId = BigInt(rawOldId).intValue
+ logger.info(this, s"invokerReg: invoker $name was assigned its previous invokerId $oldId")
+ oldId
+ }
+ } else {
+ val newId = overwriteId.getOrElse("")
+ zkClient.create().orSetData().forPath(myIdPath, BigInt(newId).toByteArray)
Review comment:
How about removing the obsolete data from the zookeeper when overwriting the ID?
Maybe we can iterate the mappings and figure out the invoker with the overwritten ID.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org