Merge lp:~rhettg/gearmand/no-unique into lp:gearmand/1.0

[rhettg]

[rhettg (rhettg)]

Queuing background jobs without a unique key is fundamentally broken with all types of persistence.

This leads to confusing behavior where jobs are accepted sometimes, but refused other times because of the underlying schema of the persistence layer.

You can see this behavior by using the special (and undocumented ?) "-" unique key, as well as the empty string "".

This change modifies the behavior to require a unique key when using persistence and providing a useful error message.

[Brian Aker (brianaker)]

Thanks! I believe you are right. I will try to take a look at this tomorrow.

[rhettg (rhettg)]

Oh, somebody did see this :)

I'm also concerned that the running daemon considers the unique key to
essentially be (task name, unique key) but the persistence
implementations don't care about task name. But one thing at a
time....

Rhett

On Mon, Jan 18, 2010 at 11:58 PM, Brian Aker <email address hidden> wrote:
> Thanks! I believe you are right. I will try to take a look at this tomorrow.
>
>
> --
> https://code.launchpad.net/~rhettg/gearmand/no-unique/+merge/16603
> You are the owner of lp:~rhettg/gearmand/no-unique.
>

=== modified file 'libgearman-server/job.c'
--- libgearman-server/job.c	2009-10-29 22:01:39 +0000
+++ libgearman-server/job.c	2009-12-26 22:19:09 +0000
@@ -63,6 +63,17 @@
     return NULL;
   }
 
+  if (server_client == NULL && server->queue_add_fn != NULL)
+  {
+    if ((unique_size == 0) || (unique_size == 1 && *unique == '-'))
+    {
+      /* If we are using persistance, we HAVE to have a unique key provided by the client */
+      GEARMAN_SERVER_ERROR(server, "Job requires a unique key")
+      *ret_ptr= GEARMAN_JOB_UNIQUE_REQUIRED;
+      return NULL;
+    }
+  }
+
   if (unique_size == 0)
   {
     server_job= NULL;

=== modified file 'libgearman/constants.h'
--- libgearman/constants.h	2009-10-29 20:18:00 +0000
+++ libgearman/constants.h	2009-12-26 22:19:09 +0000
@@ -95,6 +95,7 @@
   GEARMAN_IGNORE_PACKET,
   GEARMAN_UNKNOWN_OPTION,
   GEARMAN_TIMEOUT,
+  GEARMAN_JOB_UNIQUE_REQUIRED,
   GEARMAN_MAX_RETURN /* Always add new error code before */
 } gearman_return_t;
 

=== modified file 'tests/sqlite_test.c'
--- tests/sqlite_test.c	2009-12-18 19:25:00 +0000
+++ tests/sqlite_test.c	2009-12-26 22:19:09 +0000
@@ -34,6 +34,7 @@
 
 /* Prototypes */
 test_return queue_add(void *object);
+test_return queue_add_no_unique(void *object);
 test_return queue_worker(void *object);
 
 void *create(void *object);
@@ -114,6 +115,37 @@
   return TEST_SUCCESS;
 }
 
+test_return queue_add_no_unique(void *object)
+{
+  worker_test_st *test= (worker_test_st *)object;
+  gearman_client_st client;
+  char job_handle[GEARMAN_JOB_HANDLE_SIZE];
+  uint8_t *value= (uint8_t *)"background_test";
+  size_t value_length= strlen("background_test");
+
+  test->run_worker= false;
+
+  if (gearman_client_create(&client) == NULL)
+    return TEST_FAILURE;
+
+  if (gearman_client_add_server(&client, NULL,
+                                WORKER_TEST_PORT) != GEARMAN_SUCCESS)
+  {
+    return TEST_FAILURE;
+  }
+
+  if (gearman_client_do_background(&client, "queue_test", "", value,
+                                   value_length, job_handle) != GEARMAN_LOST_CONNECTION)
+  {
+    return TEST_FAILURE;
+  }
+
+  gearman_client_free(&client);
+
+  test->run_worker= true;
+  return TEST_SUCCESS;
+}
+
 
 test_return flush(void)
 {
@@ -167,6 +199,7 @@
 test_st tests[] ={
   {"add", 0, queue_add },
   {"worker", 0, queue_worker },
+  {"add_no_unique", 0, queue_add_no_unique },
   {0, 0, 0}
 };
 

=== modified file 'tests/sqlite_test.rec'
--- tests/sqlite_test.rec	2009-07-16 20:45:40 +0000
+++ tests/sqlite_test.rec	2009-12-26 22:19:09 +0000
@@ -5,6 +5,7 @@
 
 Testing add                                               [ ok     ]
 Testing worker                                            [ ok     ]
+Testing add_no_unique                                     [ ok     ]
 
 ==========================================================================