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

[HackMan]

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.

[Brian Aker (brianaker)]

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)]

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

[HackMan (mm-yuhu)]

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)]

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)]

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)]

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)]

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)]

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)]

Any word on this?

Unmerged revisions

332. By HackMan on 2010-03-11

Added rmfunc server command

=== 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)