Review comments

Shopify · Jul 4, 2019 · 694b187 · 694b187
1 parent 8a191f1
commit 694b187
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 9 deletions.
diff --git a/README.md b/README.md
@@ -187,7 +187,7 @@ Per the above example, you no longer need to care about the number of tickets. R
 
 In this case, we'd allow 49% of the workers on a particular host to connect to this redis resource.
 
-In particular, 1 worker = 1 ticket (due to `ceil`), 2 workers = 2 tickets (due to `min_tickets`), 4 workers = 2 tickets, 16 workers = 8 tickets, 100 workers = 49 tickets.
+In particular, 1 worker = 1 ticket (due to `ceil`), 2 workers = 2 tickets (due to `min_tickets`), 4 workers = 2 tickets (due to `ceil`), 100 workers = 49 tickets.
 
 **Note**:
 

diff --git a/ext/semian/resource.c b/ext/semian/resource.c
@@ -4,7 +4,7 @@ static VALUE
 cleanup_semian_resource_acquire(VALUE self);
 
 static void
-check_tickets_xor_quota_arg(VALUE tickets, VALUE quota);
+check_tickets_xor_quota_arg(VALUE tickets, VALUE min_tickets, VALUE quota);
 
 static double
 check_quota_arg(VALUE quota);
@@ -206,7 +206,7 @@ VALUE
 semian_resource_initialize(VALUE self, VALUE id, VALUE tickets, VALUE quota, VALUE permissions, VALUE default_timeout, VALUE min_tickets)
 {
   // Check and cast arguments
-  check_tickets_xor_quota_arg(tickets, quota);
+  check_tickets_xor_quota_arg(tickets, min_tickets, quota);
   double c_quota = check_quota_arg(quota);
   int c_tickets = check_tickets_arg(tickets);
   long c_permissions = check_permissions_arg(permissions);
@@ -257,10 +257,22 @@ check_permissions_arg(VALUE permissions)
 }
 
 static void
-check_tickets_xor_quota_arg(VALUE tickets, VALUE quota)
+check_tickets_xor_quota_arg(VALUE tickets, VALUE min_tickets, VALUE quota)
 {
-  if ((TYPE(tickets) == T_NIL && TYPE(quota) == T_NIL) ||(TYPE(tickets) != T_NIL && TYPE(quota) != T_NIL)){
-    rb_raise(rb_eArgError, "Must pass exactly one of ticket or quota");
+  const char *msg = "Must pass exactly one of ticket or quota/min_tickets";
+  if (TYPE(quota) != T_NIL) {
+    if (TYPE(tickets) != T_NIL) {
+      dprintf("FOO");
+      rb_raise(rb_eArgError, msg);
+    }
+  } else if (TYPE(tickets) != T_NIL) {
+    if (TYPE(quota) != T_NIL || TYPE(min_tickets) != T_NIL) {
+      dprintf("FOO");
+      rb_raise(rb_eArgError, msg);
+    }
+  } else {
+      dprintf("FOO");
+    rb_raise(rb_eArgError, msg);
   }
 }
 

diff --git a/ext/semian/tickets.c b/ext/semian/tickets.c
@@ -84,6 +84,6 @@ calculate_quota_tickets(int sem_id, double quota, int min_tickets)
 {
   int workers = get_sem_val(sem_id, SI_SEM_REGISTERED_WORKERS);
   int tickets = (int) ceil(workers * quota);
-  dprintf("Calculating quota tickets - sem_id:%d quota:%0.2f%% workers:%d min_tickets:%d tickets:%d", sem_id, quota, workers, min_tickets, tickets);
+  dprintf("Calculating quota tickets - sem_id:%d quota:%0.2f%% workers:%d min_tickets:%d tickets:%d", sem_id, quota * 100.0, workers, min_tickets, tickets);
   return min_tickets > 0 ? min(workers, max(tickets, min_tickets)) : tickets;
 }
diff --git a/ext/semian/util.h b/ext/semian/util.h
@@ -5,7 +5,7 @@
 #include <stdio.h>
 #include <time.h>
 
-#if defined(DEBUG) || defined(SEMIAN_DEBUG)
+#ifdef DEBUG
 #  define DEBUG_TEST 1
 #else
 #  define DEBUG_TEST 0

diff --git a/lib/semian/resource.rb b/lib/semian/resource.rb
@@ -9,7 +9,7 @@ def instance(*args)
       end
     end
 
-    def initialize(name, tickets: nil, quota: nil, permissions: 0660, timeout: 0, min_tickets: 1)
+    def initialize(name, tickets: nil, quota: nil, permissions: 0660, timeout: 0, min_tickets: nil)
       if Semian.semaphores_enabled?
         initialize_semaphore(name, tickets, quota, permissions, timeout, min_tickets) if respond_to?(:initialize_semaphore)
       else