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

Proposed by rhettg on 2009-12-26
Status: Merged
Merged at revision: not available
Proposed branch: lp:~rhettg/gearmand/no-unique
Merge into: lp:gearmand/1.0
Diff against target: 102 lines (+46/-0)
4 files modified
libgearman-server/job.c (+11/-0)
libgearman/constants.h (+1/-0)
tests/sqlite_test.c (+33/-0)
tests/sqlite_test.rec (+1/-0)
To merge this branch: bzr merge lp:~rhettg/gearmand/no-unique
Reviewer Review Type Date Requested Status
Gearman-developers 2009-12-26 Pending
Review via email: mp+16603@code.launchpad.net
To post a comment you must log in.
rhettg (rhettg) wrote :

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) wrote :

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

rhettg (rhettg) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libgearman-server/job.c'
2--- libgearman-server/job.c 2009-10-29 22:01:39 +0000
3+++ libgearman-server/job.c 2009-12-26 22:19:09 +0000
4@@ -63,6 +63,17 @@
5 return NULL;
6 }
7
8+ if (server_client == NULL && server->queue_add_fn != NULL)
9+ {
10+ if ((unique_size == 0) || (unique_size == 1 && *unique == '-'))
11+ {
12+ /* If we are using persistance, we HAVE to have a unique key provided by the client */
13+ GEARMAN_SERVER_ERROR(server, "Job requires a unique key")
14+ *ret_ptr= GEARMAN_JOB_UNIQUE_REQUIRED;
15+ return NULL;
16+ }
17+ }
18+
19 if (unique_size == 0)
20 {
21 server_job= NULL;
22
23=== modified file 'libgearman/constants.h'
24--- libgearman/constants.h 2009-10-29 20:18:00 +0000
25+++ libgearman/constants.h 2009-12-26 22:19:09 +0000
26@@ -95,6 +95,7 @@
27 GEARMAN_IGNORE_PACKET,
28 GEARMAN_UNKNOWN_OPTION,
29 GEARMAN_TIMEOUT,
30+ GEARMAN_JOB_UNIQUE_REQUIRED,
31 GEARMAN_MAX_RETURN /* Always add new error code before */
32 } gearman_return_t;
33
34
35=== modified file 'tests/sqlite_test.c'
36--- tests/sqlite_test.c 2009-12-18 19:25:00 +0000
37+++ tests/sqlite_test.c 2009-12-26 22:19:09 +0000
38@@ -34,6 +34,7 @@
39
40 /* Prototypes */
41 test_return queue_add(void *object);
42+test_return queue_add_no_unique(void *object);
43 test_return queue_worker(void *object);
44
45 void *create(void *object);
46@@ -114,6 +115,37 @@
47 return TEST_SUCCESS;
48 }
49
50+test_return queue_add_no_unique(void *object)
51+{
52+ worker_test_st *test= (worker_test_st *)object;
53+ gearman_client_st client;
54+ char job_handle[GEARMAN_JOB_HANDLE_SIZE];
55+ uint8_t *value= (uint8_t *)"background_test";
56+ size_t value_length= strlen("background_test");
57+
58+ test->run_worker= false;
59+
60+ if (gearman_client_create(&client) == NULL)
61+ return TEST_FAILURE;
62+
63+ if (gearman_client_add_server(&client, NULL,
64+ WORKER_TEST_PORT) != GEARMAN_SUCCESS)
65+ {
66+ return TEST_FAILURE;
67+ }
68+
69+ if (gearman_client_do_background(&client, "queue_test", "", value,
70+ value_length, job_handle) != GEARMAN_LOST_CONNECTION)
71+ {
72+ return TEST_FAILURE;
73+ }
74+
75+ gearman_client_free(&client);
76+
77+ test->run_worker= true;
78+ return TEST_SUCCESS;
79+}
80+
81
82 test_return flush(void)
83 {
84@@ -167,6 +199,7 @@
85 test_st tests[] ={
86 {"add", 0, queue_add },
87 {"worker", 0, queue_worker },
88+ {"add_no_unique", 0, queue_add_no_unique },
89 {0, 0, 0}
90 };
91
92
93=== modified file 'tests/sqlite_test.rec'
94--- tests/sqlite_test.rec 2009-07-16 20:45:40 +0000
95+++ tests/sqlite_test.rec 2009-12-26 22:19:09 +0000
96@@ -5,6 +5,7 @@
97
98 Testing add [ ok ]
99 Testing worker [ ok ]
100+Testing add_no_unique [ ok ]
101
102 ==========================================================================
103

Subscribers

People subscribed via source and target branches

to all changes: