[GitHub] [helix] rahulrane50 commented on a diff in pull request #2515: Distributed Semaphore Implementation

["rahulrane50 (via GitHub)" <gi...@apache.org>]

rahulrane50 commented on code in PR #2515:
URL: https://github.com/apache/helix/pull/2515#discussion_r1237409721


##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/lock/DistributedSemaphore.java:
##########
@@ -50,23 +87,50 @@ public DistributedSemaphore(MetaClientInterface<DataRecord> client) {
    * @param capacity capacity of the semaphore
    */
   public void createSemaphore(String path, int capacity) {
-    throw new NotImplementedException("Not implemented yet.");
+    if (capacity <= 0) {
+      throw new MetaClientException("Capacity must be positive");
+    }
+    if (path == null || path.isEmpty()) {
+      throw new MetaClientException("Invalid path to create semaphore");
+    }
+    if (_metaClient.exists(path) != null) {
+      throw new MetaClientException("Semaphore already exists");
+    }
+    _path = path;

Review Comment:
   Shouldn't we do this after line 104? What if creation of znode fails?



##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/lock/DistributedSemaphore.java:
##########
@@ -75,8 +133,19 @@ public Permit acquire() {
    * @param count number of permits to acquire
    * @return a collection of permits
    */
-  public Collection<Permit> acquire(int count) {
-    throw new NotImplementedException("Not implemented yet.");
+  public synchronized Collection<Permit> acquire(int count) {
+    if (getRemainingCapacity() < count) {
+      LOG.warn("No sufficient permits available. Attempt to acquire {} permits, but only {} permits available", count,
+          getRemainingCapacity());
+      return null;
+    } else {
+      updateAcquirePermit(count);

Review Comment:
   I'm still trying to understand how current implementation is atomic? We check and update znode with remaining capacity in updateAcquirePermit (nit :i feel it should be renamed to checkAcquistionAndUpdatePermit or something) and then update in-memory object in retrievePermit() but it's not atomic right? There still could be race conditions here. For ex there are two acquire calls with different numbers then caller may get different values based on order of execution.



-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org