[GitHub] jiangpengcheng commented on a change in pull request #3656: Adjust the behavior when update controller cluster

[GitBox <gi...@apache.org>]

jiangpengcheng commented on a change in pull request #3656: Adjust the behavior when update controller cluster
URL: https://github.com/apache/incubator-openwhisk/pull/3656#discussion_r220066934
 
 

 ##########
 File path: tests/src/test/scala/whisk/common/ForcibleSemaphoreTests.scala
 ##########
 @@ -85,4 +85,18 @@ class ForcibleSemaphoreTests extends FlatSpec with Matchers {
       acquires should contain theSameElementsAs result
     }
   }
+
+  it should "set the max allowed permits dynamically" in {
+    val s = new ForcibleSemaphore(10)
+    s.tryAcquire(2) shouldBe true // 8 permits left
+
+    s.setMaxPermits(5) // reduce max permits
+    s.availablePermits shouldBe 3
+
+    s.setMaxPermits(10) // increase max permits
+    s.availablePermits shouldBe 8
+
+    s.setMaxPermits(10) // nothing changed
+    s.availablePermits shouldBe 8
 
 Review comment:
   @tysonnorris 
   
   - for the `if delta < 0 =>`, I think the `s.setMaxPermits(5) // reduce max permits` match the case
   
   - for the concurrent situations, I added below tests
   
   ```
     it should "set the max allowed permits while call release concurrently" in {
       (0 until 100).foreach { _ =>
         val s = new ForcibleSemaphore(32)
         s.tryAcquire(32) // 0 permits left
   
         (0 until 64).par.map{ i =>
           if ( i == 16)
             s.setMaxPermits(16) // force acquire 16 permits
           else
             s.release()
         }
   
         s.availablePermits shouldBe 47 // 0 - 16 + 63
       }
   
       (0 until 100).foreach { _ =>
         val s = new ForcibleSemaphore(32)
         s.tryAcquire(32) // 0 permits left
   
         (0 until 64).par.map{ i =>
           if ( i == 16)
             s.setMaxPermits(64) // release 32 permits
           else
             s.release()
         }
   
         s.availablePermits shouldBe 95 // 0 + 32 + 63
       }
     }
   
     it should "set the max allowed permits while call forceAcquire concurrently" in {
       (0 until 100).foreach { _ =>
         val s = new ForcibleSemaphore(32)
   
         (0 until 64).par.map{ i =>
           if ( i == 16)
             s.setMaxPermits(16) // force acquire 16 permits
           else
             s.forceAcquire()
         }
   
         s.availablePermits shouldBe -47 // 32 - 16 - 63
       }
       (0 until 100).foreach { _ =>
         val s = new ForcibleSemaphore(32)
   
         (0 until 64).par.map{ i =>
           if ( i == 16)
             s.setMaxPermits(64) // release 32 permits
           else
             s.forceAcquire()
         }
   
         s.availablePermits shouldBe 1 // 32 + 32 - 63
       }
     }
   
     it should "set the max allowed permits while call tryAcquire concurrently" in {
       (0 until 100).foreach { _ =>
         val s = new ForcibleSemaphore(32)
   
         (0 until 64).par.map{ i =>
           if ( i == 16)
             s.setMaxPermits(16) // force acquire 16 permits
           else
             s.tryAcquire()
         }
   
         s.availablePermits shouldBe ??? // not sure how many tryAcquire succeed
       }
   
       (0 until 100).foreach { _ =>
         val s = new ForcibleSemaphore(32)
   
         (0 until 64).par.map{ i =>
           if ( i == 16)
             s.setMaxPermits(64) // release 32 permits
           else
             s.tryAcquire()
         }
   
         s.availablePermits shouldBe ??? // not sure how many tryAcquire succeed
       }
     }
   ```
   
   the first two tests will succeed while the last one will not, I believe the reason is that the `nonFairTryAcquireShared` will not change the state while `available - acquires < 0`, so we can not make sure a `tryAcquire` is success or not while `setMaxPermits` is in flight, any advice?
   
   ```
       @tailrec
       final def nonFairTryAcquireShared(acquires: Int): Int = {
         val available = getState
         val remaining = available - acquires
         if (remaining < 0 || compareAndSetState(available, remaining)) {
           remaining
         } else {
           nonFairTryAcquireShared(acquires)
         }
       }
   ```

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services