Merge lp:~mm-yuhu/gearmand/server-funcs into lp:gearmand/1.0

Proposed by HackMan on 2010-03-11
Status: Rejected
Rejected by: Brian Aker on 2010-06-15
Proposed branch: lp:~mm-yuhu/gearmand/server-funcs
Merge into: lp:gearmand/1.0
Diff against target: 38 lines (+28/-0)
1 file modified
libgearman-server/server.c (+28/-0)
To merge this branch: bzr merge lp:~mm-yuhu/gearmand/server-funcs
Reviewer Review Type Date Requested Status
Gearman-developers 2010-03-11 Pending
Review via email: mp+21142@code.launchpad.net

Description of the change

Every now and then during development one hit a situation where a client connects to a job server and requests a function to be executed. If the client uses dispatch_background then the server will wait depending on the configuration for a worker to do the job.

However in my case the client has issued wrong arguments and I don't want to connect the worker before removing the wrong requests and I don't want to restart the job server just for that.

This patch introduces a new command rmfunc which has only one argument, the function name that we want to remove. If there are connected workers, for this function we do not allow removing of the function.

To post a comment you must log in.
Brian Aker (brianaker) wrote :

Hi!

A couple of things:

1) strcasecmp() is not found on all platforms.

2) We need a test case.

Thanks!
 -Brian

On Mar 11, 2010, at 5:22 AM, HackMan wrote:

> HackMan has proposed merging lp:~mm-yuhu/gearmand/mm-dev into
> lp:gearmand.
>
> Requested reviews:
> Gearman-developers (gearman-developers)
>
>
> Every now and then during development one hit a situation where a
> client connects to a job server and requests a function to be
> executed. If the client uses dispatch_background then the server
> will wait depending on the configuration for a worker to do the job.
>
> However in my case the client has issued wrong arguments and I don't
> want to connect the worker before removing the wrong requests and I
> don't want to restart the job server just for that.
>
> This patch introduces a new command rmfunc which has only one
> argument, the function name that we want to remove. If there are
> connected workers, for this function we do not allow removing of the
> function.
> --
> https://code.launchpad.net/~mm-yuhu/gearmand/mm-dev/+merge/21142
> Your team Gearman-developers is subscribed to branch lp:gearmand.
> === modified file 'libgearman-server/server.c'
> --- libgearman-server/server.c 2010-01-28 21:45:14 +0000
> +++ libgearman-server/server.c 2010-03-11 13:22:17 +0000
> @@ -849,6 +849,34 @@
> if (size < total)
> snprintf(data + size, total - size, ".\n");
> }
> + else if (!strcasecmp("rmfunc", (char *)(packet->arg[0])))
> + {
> + if (packet->argc == 1)
> + {
> + snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "ERR
> incomplete_args "
> + "An+incomplete+set+of+arguments+was+sent+to+this+command\n");
> + }
> + else
> + {
> + for (function= server_con->thread->server->function_list;
> + function != NULL; function= function->next)
> + {
> + if (!strcasecmp(function->function_name, (char *)(packet-
> >arg[1])))
> + {
> + if ( function->worker_count == 0 )
> + {
> + gearman_server_function_free(function);
> + snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "OK\n");
> + }
> + else
> + {
> + snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "ERR there
> are still connected workers\n");
> + }
> + }
> + }
> + }
> + }
> +
> else if (!strcasecmp("maxqueue", (char *)(packet->arg[0])))
> {
> if (packet->argc == 1)
>

Eric Day (eday) wrote :
Download full text (3.3 KiB)

Hey!

Use of strcasecmp is not an issue, it's already there in the other
text parsing commands and we've not hit problems on any platform we
care about yet.

The bigger issue here is that jobs that are in the queue for a
function being removed will have dangling pointers. For example,
even with no workers, there may still be clients with jobs that
reference the function objects. These need to be checked and either
also removed, which means responding to client connections that some
error has occurred with the job, or simply not deleting the function
if jobs exist.

-Eric

On Thu, Mar 11, 2010 at 04:57:16PM -0000, Brian Aker wrote:
> Hi!
>
> A couple of things:
>
> 1) strcasecmp() is not found on all platforms.
>
> 2) We need a test case.
>
> Thanks!
> -Brian
>
> On Mar 11, 2010, at 5:22 AM, HackMan wrote:
>
> > HackMan has proposed merging lp:~mm-yuhu/gearmand/mm-dev into
> > lp:gearmand.
> >
> > Requested reviews:
> > Gearman-developers (gearman-developers)
> >
> >
> > Every now and then during development one hit a situation where a
> > client connects to a job server and requests a function to be
> > executed. If the client uses dispatch_background then the server
> > will wait depending on the configuration for a worker to do the job.
> >
> > However in my case the client has issued wrong arguments and I don't
> > want to connect the worker before removing the wrong requests and I
> > don't want to restart the job server just for that.
> >
> > This patch introduces a new command rmfunc which has only one
> > argument, the function name that we want to remove. If there are
> > connected workers, for this function we do not allow removing of the
> > function.
> > --
> > https://code.launchpad.net/~mm-yuhu/gearmand/mm-dev/+merge/21142
> > Your team Gearman-developers is subscribed to branch lp:gearmand.
> > === modified file 'libgearman-server/server.c'
> > --- libgearman-server/server.c 2010-01-28 21:45:14 +0000
> > +++ libgearman-server/server.c 2010-03-11 13:22:17 +0000
> > @@ -849,6 +849,34 @@
> > if (size < total)
> > snprintf(data + size, total - size, ".\n");
> > }
> > + else if (!strcasecmp("rmfunc", (char *)(packet->arg[0])))
> > + {
> > + if (packet->argc == 1)
> > + {
> > + snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "ERR
> > incomplete_args "
> > + "An+incomplete+set+of+arguments+was+sent+to+this+command\n");
> > + }
> > + else
> > + {
> > + for (function= server_con->thread->server->function_list;
> > + function != NULL; function= function->next)
> > + {
> > + if (!strcasecmp(function->function_name, (char *)(packet-
> > >arg[1])))
> > + {
> > + if ( function->worker_count == 0 )
> > + {
> > + gearman_server_function_free(function);
> > + snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "OK\n");
> > + }
> > + else
> > + {
> > + snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "ERR there
> > are still connected workers\n");
> > + }
> > + }
> > + }
> > + }
> > + }
> > +
> > else if (!strcasecmp("maxqueue", (char *)(packet->arg[0...

Read more...

HackMan (mm-yuhu) wrote :

I used strcasecmp since this is how the status command was implemented. If you want I can rewrite it using a combination of to lower and strcmp.

About the test case... I'm writing it right now it will be ready within the next few hours.

Brian Aker (brianaker) wrote :

Hi!

On Mar 11, 2010, at 10:36 AM, HackMan wrote:

> I used strcasecmp since this is how the status command was
> implemented. If you want I can rewrite it using a combination of to
> lower and strcmp.

The bigger issue is that if someone tosses a UTF-8 string in, we will
incorrectly lowercase it (which... we aren't perfect on this already).

The test case is more important :)

Cheers,
 -Brian

Eric Day (eday) wrote :

The language/charset issues with strcasecmp are irrelevant here because
it's only used for predefined server commands, nothing that is user
defined. Since strcasecmp is used in code immediately surrounding it
now, it should also be used here for consistency. If there is reason
to remove the use of it, that should be done everywhere, probably in
a different branch.

-Eric

On Thu, Mar 11, 2010 at 06:54:15PM -0000, Brian Aker wrote:
> Hi!
>
> On Mar 11, 2010, at 10:36 AM, HackMan wrote:
>
> > I used strcasecmp since this is how the status command was
> > implemented. If you want I can rewrite it using a combination of to
> > lower and strcmp.
>
> The bigger issue is that if someone tosses a UTF-8 string in, we will
> incorrectly lowercase it (which... we aren't perfect on this already).
>
> The test case is more important :)
>
> Cheers,
> -Brian
> --
> https://code.launchpad.net/~mm-yuhu/gearmand/server-funcs/+merge/21142
> Your team Gearman-developers is subscribed to branch lp:gearmand.

Brian Aker (brianaker) wrote :

Hi!

On Mar 11, 2010, at 11:18 AM, Eric Day wrote:

> it's only used for predefined server commands, nothing that is user

I am talking about in general our usage of strings.

Cheers,
 -Brian

Eric Day (eday) wrote :

Ok, cool. I was just trying be clear for this specific merge
proposal. :)

-Eric

On Thu, Mar 11, 2010 at 07:45:25PM -0000, Brian Aker wrote:
> Hi!
>
> On Mar 11, 2010, at 11:18 AM, Eric Day wrote:
>
> > it's only used for predefined server commands, nothing that is user
>
> I am talking about in general our usage of strings.
>
> Cheers,
> -Brian
> --
> https://code.launchpad.net/~mm-yuhu/gearmand/server-funcs/+merge/21142
> Your team Gearman-developers is subscribed to branch lp:gearmand.

HackMan (mm-yuhu) wrote :

I'm ready with the test case, however It will take some time to implement the job freeing so I hope that on Monday I will have all required.

Brian Aker (brianaker) wrote :

Any word on this?

Unmerged revisions

332. By HackMan on 2010-03-11

Added rmfunc server command

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libgearman-server/server.c'
2--- libgearman-server/server.c 2010-01-28 21:45:14 +0000
3+++ libgearman-server/server.c 2010-03-11 13:22:17 +0000
4@@ -849,6 +849,34 @@
5 if (size < total)
6 snprintf(data + size, total - size, ".\n");
7 }
8+ else if (!strcasecmp("rmfunc", (char *)(packet->arg[0])))
9+ {
10+ if (packet->argc == 1)
11+ {
12+ snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "ERR incomplete_args "
13+ "An+incomplete+set+of+arguments+was+sent+to+this+command\n");
14+ }
15+ else
16+ {
17+ for (function= server_con->thread->server->function_list;
18+ function != NULL; function= function->next)
19+ {
20+ if (!strcasecmp(function->function_name, (char *)(packet->arg[1])))
21+ {
22+ if ( function->worker_count == 0 )
23+ {
24+ gearman_server_function_free(function);
25+ snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "OK\n");
26+ }
27+ else
28+ {
29+ snprintf(data, GEARMAN_TEXT_RESPONSE_SIZE, "ERR there are still connected workers\n");
30+ }
31+ }
32+ }
33+ }
34+ }
35+
36 else if (!strcasecmp("maxqueue", (char *)(packet->arg[0])))
37 {
38 if (packet->argc == 1)

Subscribers

People subscribed via source and target branches

to all changes: