Avro
  1. Avro
  2. AVRO-544

Allow the HttpServer to serve forever without a call to Thread.sleep()

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4.0
    • Component/s: java
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Changed servers to not be started by constructor, but by new start() method.

      Description

      One way would be to expose the join() method on the HttpServer: http://jetty.codehaus.org/jetty/jetty-6/apidocs/org/mortbay/jetty/Server.html#join%28%29

      1. AVRO-544.patch
        0.4 kB
        Jeff Hammerbacher
      2. AVRO-544-2.patch
        0.5 kB
        Jeff Hammerbacher
      3. AVRO-544-3.patch
        5 kB
        Jeff Hammerbacher
      4. AVRO-544-4.patch
        5 kB
        Jeff Hammerbacher
      5. AVRO-544.patch
        16 kB
        Doug Cutting

        Issue Links

          Activity

          Hide
          Jeff Hammerbacher added a comment -

          Hey Patrick,

          Thanks for the idea. I did the dead-simplest exposing of join() and it works for me. Now I can do things like:

              HttpServer server = new HttpServer(r, 9090);
              server.join();
          

          instead of:

              HttpServer server = new HttpServer(r, 9090);
              Thread.sleep(10000);
          

          Which is sane. I could be naive in how I expose join(), however, so if someone more familiar with Java servers wants to take a look, please do.

          Thanks,
          Jeff

          Show
          Jeff Hammerbacher added a comment - Hey Patrick, Thanks for the idea. I did the dead-simplest exposing of join() and it works for me. Now I can do things like: HttpServer server = new HttpServer(r, 9090); server.join(); instead of: HttpServer server = new HttpServer(r, 9090); Thread .sleep(10000); Which is sane. I could be naive in how I expose join(), however, so if someone more familiar with Java servers wants to take a look, please do. Thanks, Jeff
          Hide
          Scott Carey added a comment -

          +1 It looks sane to expose Jetty's join() method. Since this is public we probably want a line of javadoc along the lines of "Will block until the server has stopped or the thread is interrupted."

          If anything more complicated is needed down the road (for example, a timeout while waiting or callback when it finishes) we can add that later.

          Show
          Scott Carey added a comment - +1 It looks sane to expose Jetty's join() method. Since this is public we probably want a line of javadoc along the lines of "Will block until the server has stopped or the thread is interrupted." If anything more complicated is needed down the road (for example, a timeout while waiting or callback when it finishes) we can add that later.
          Hide
          Jeff Hammerbacher added a comment -

          Adding a Javadoc per Scott's feedback

          Show
          Jeff Hammerbacher added a comment - Adding a Javadoc per Scott's feedback
          Hide
          Ken Krugler added a comment -

          In all my code where I need to start up a Jetty server, I do the start() and join() together, as in:

          public void start() throws Exception

          { _server.start(); _server.join(); }

          What's the rational for splitting these up? If you need to have explicit control over starting/stopping the server, then I would have a start() method as per above, otherwise just put both into the constructor.

          Show
          Ken Krugler added a comment - In all my code where I need to start up a Jetty server, I do the start() and join() together, as in: public void start() throws Exception { _server.start(); _server.join(); } What's the rational for splitting these up? If you need to have explicit control over starting/stopping the server, then I would have a start() method as per above, otherwise just put both into the constructor.
          Hide
          Jeff Hammerbacher added a comment - - edited

          For another point of reference, rather than calling start() in the ctor, org.apache.hadoop.http.HttpServer exposes start(), stop(), and join() as public methods.

          I think I like the Hadoop way best: pull start() out of the ctor and make it a public method. That means existing client code needs to change to call start() after the ctor, but I think it's worth it.

          Thoughts?

          Show
          Jeff Hammerbacher added a comment - - edited For another point of reference, rather than calling start() in the ctor, org.apache.hadoop.http.HttpServer exposes start() , stop() , and join() as public methods. I think I like the Hadoop way best: pull start() out of the ctor and make it a public method. That means existing client code needs to change to call start() after the ctor, but I think it's worth it. Thoughts?
          Hide
          Ken Krugler added a comment -

          If you're requiring a start() method call (which I like), then I'm still curious why you think having join() is important.

          Show
          Ken Krugler added a comment - If you're requiring a start() method call (which I like), then I'm still curious why you think having join() is important.
          Hide
          Scott Carey added a comment -

          +1 on AVRO-544-2.patch

          What's the rational for splitting these up? If you need to have explicit control over starting/stopping the server, then I would have a start() method as per above, otherwise just put both into the constructor.

          start() should not block. The constructor should most definitely not block. If a user wants to block, there is join(), they should not be forced to block and burn up a thread.

          Use cases I've had where I don't want to block are numerous.

          Here is an example, two servers with different ports / configurations plus a thread pool with scheduled tasks for custom business logic:

          MyCustomExecutorService pool = ...
          HttpServer server1 = ...
          HttpServer server2 = ...
          
          pool.submitWithFixedDelay(someTask);
          server1.start();
          server2.start();
          
          boolean shutdown = false;
          while(!shutdown) {
            shutdown = MyCustomShutdownListener.awaitShutdown();
          }
          server2.stop();
          server1.stop();
          pool.shutdownNow();
          

          Also, it is generally better to explicitly start asynchronous activities after construction and configuration of their representative entities – there are many use cases where construction and starting should be separated.

          Show
          Scott Carey added a comment - +1 on AVRO-544 -2.patch What's the rational for splitting these up? If you need to have explicit control over starting/stopping the server, then I would have a start() method as per above, otherwise just put both into the constructor. start() should not block. The constructor should most definitely not block. If a user wants to block, there is join(), they should not be forced to block and burn up a thread. Use cases I've had where I don't want to block are numerous. Here is an example, two servers with different ports / configurations plus a thread pool with scheduled tasks for custom business logic: MyCustomExecutorService pool = ... HttpServer server1 = ... HttpServer server2 = ... pool.submitWithFixedDelay(someTask); server1.start(); server2.start(); boolean shutdown = false ; while (!shutdown) { shutdown = MyCustomShutdownListener.awaitShutdown(); } server2.stop(); server1.stop(); pool.shutdownNow(); Also, it is generally better to explicitly start asynchronous activities after construction and configuration of their representative entities – there are many use cases where construction and starting should be separated.
          Hide
          Scott Carey added a comment -

          I think I like the Hadoop way best: pull start() out of the ctor and make it a public method. That means existing client code needs to change to call start() after the ctor, but I think it's worth it.

          +1 Oops, I didn't interpret the whole thread here properly the first time through. A new patch that did this like Hadoop does would be better than the current one IMO.

          I think we should probably avoid doing anything other than prep and configuration in the constructor and allow users to control when the server is started. For example it is often important to only open your ports for requests after some prep-work is done, and it is often useful to construct the object at a different time or place from when you start.

          Exposing start(), stop() and join() like Hadoop is the way to go – it lets users be maximally flexible. There is a reason why the Jetty API has it like that.

          We can consider a convenience method that does both start() and join() in one go (perhaps 'run()'?) but that is trivial and some might call it API clutter. It saves one line of trivial code, so I'm a little dubious of the value.
          Lets finish this JIRA and discuss that in another JIRA. Ken, if its important to you to avoid typing both start() and join(), please open a different JIRA for that.

          Show
          Scott Carey added a comment - I think I like the Hadoop way best: pull start() out of the ctor and make it a public method. That means existing client code needs to change to call start() after the ctor, but I think it's worth it. +1 Oops, I didn't interpret the whole thread here properly the first time through. A new patch that did this like Hadoop does would be better than the current one IMO. I think we should probably avoid doing anything other than prep and configuration in the constructor and allow users to control when the server is started. For example it is often important to only open your ports for requests after some prep-work is done, and it is often useful to construct the object at a different time or place from when you start. Exposing start(), stop() and join() like Hadoop is the way to go – it lets users be maximally flexible. There is a reason why the Jetty API has it like that. We can consider a convenience method that does both start() and join() in one go (perhaps 'run()'?) but that is trivial and some might call it API clutter. It saves one line of trivial code, so I'm a little dubious of the value. Lets finish this JIRA and discuss that in another JIRA. Ken, if its important to you to avoid typing both start() and join(), please open a different JIRA for that.
          Hide
          Ken Krugler added a comment -

          Ken, if its important to you to avoid typing both start() and join(), please open a different JIRA for that.

          No, I was just curious about having a separate start() versus join(), as I hadn't run into that use case.

          Show
          Ken Krugler added a comment - Ken, if its important to you to avoid typing both start() and join(), please open a different JIRA for that. No, I was just curious about having a separate start() versus join() , as I hadn't run into that use case.
          Hide
          Jeff Hammerbacher added a comment - - edited

          Okay, here's a patch that factors start() out of the ctor as well, and changes all of the users of the ctor as well. Note that exposing start() should allow us to make TestStatsPluginAndServlet.java less hacky (e.g. only start one server), but I'll leave that for those who know Java better than me.

          Also, I note that DatagramServer and SocketServer both implement the Server interface, so they should probably override start() as well. Given that they are deprecated, and they don't currently use the @Override annotation on close() either, I didn't bother to change them.

          Lastly, why do we use close()? Jetty uses stop(), and that seems like the logical thing for Avro servers to implement as well.

          Show
          Jeff Hammerbacher added a comment - - edited Okay, here's a patch that factors start() out of the ctor as well, and changes all of the users of the ctor as well. Note that exposing start() should allow us to make TestStatsPluginAndServlet.java less hacky (e.g. only start one server), but I'll leave that for those who know Java better than me. Also, I note that DatagramServer and SocketServer both implement the Server interface, so they should probably override start() as well. Given that they are deprecated, and they don't currently use the @Override annotation on close() either, I didn't bother to change them. Lastly, why do we use close()? Jetty uses stop(), and that seems like the logical thing for Avro servers to implement as well.
          Hide
          Jeff Hammerbacher added a comment -

          Lastly, why do we use close()? Jetty uses stop(), and that seems like the logical thing for Avro servers to implement as well.

          To clarify: I am only talking about choice of name for the function.

          Show
          Jeff Hammerbacher added a comment - Lastly, why do we use close()? Jetty uses stop(), and that seems like the logical thing for Avro servers to implement as well. To clarify: I am only talking about choice of name for the function.
          Hide
          Ken Krugler added a comment -

          I agree that stop() is the better name.

          Show
          Ken Krugler added a comment - I agree that stop() is the better name.
          Hide
          Jeff Hammerbacher added a comment -

          Ken: I've created a new issue for renaming close() to stop() so that this issue will not block

          Show
          Jeff Hammerbacher added a comment - Ken: I've created a new issue for renaming close() to stop() so that this issue will not block
          Hide
          Philip Zeyliger added a comment -

          Took a look via reviewboard:

          The code, besides the catch (Exception e), looks fine.

          I do worry that this is a massively backwards-incompatible change. Anyone who's set up an Avro RPC server using HttpServer now needs to add a line to their code. I can only think of hacky ways around that.

          trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java
          <http://review.hbase.org/r/281/#comment1391>

          I would add to the java doc that this throws AvroRuntimeException.

          trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java
          <http://review.hbase.org/r/281/#comment1392>

          catch (Exception e) is poor form, typically. It's usually better to explicitly catch the type of exception that Jetty's start throws.

          • Philip
          Show
          Philip Zeyliger added a comment - Took a look via reviewboard: The code, besides the catch (Exception e), looks fine. I do worry that this is a massively backwards-incompatible change. Anyone who's set up an Avro RPC server using HttpServer now needs to add a line to their code. I can only think of hacky ways around that. trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java < http://review.hbase.org/r/281/#comment1391 > I would add to the java doc that this throws AvroRuntimeException. trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java < http://review.hbase.org/r/281/#comment1392 > catch (Exception e) is poor form, typically. It's usually better to explicitly catch the type of exception that Jetty's start throws. Philip
          Hide
          Jeff Hammerbacher added a comment -

          Added a @throws comment to the Javadoc for start(). Any other comments? I think this may be ready to commit.

          Show
          Jeff Hammerbacher added a comment - Added a @throws comment to the Javadoc for start(). Any other comments? I think this may be ready to commit.
          Hide
          Philip Zeyliger added a comment -

          From reviewboard:

          Jeff Hammerbacher:

          > On 2010-07-07 16:51:01, Philip Zeyliger wrote:
          > > The code, besides the catch (Exception e), looks fine.
          > >
          > > I do worry that this is a massively backwards-incompatible change. Anyone who's set up an Avro RPC server using HttpServer now needs to add a line to their code. I can only think of hacky ways around that.

          Well, considering that Avro RPC is under active development, I think now is the time to make massively breaking changes. The discussion on this ticket seems to indicate that the changes are for the better, so I vote for making them now while we still can.

          To that end, see also https://issues.apache.org/jira/browse/AVRO-594

          Scott Carey:

          ReviewBoard is awesome. I'll need to figure out how to use it sometime.

          Comments inline:
          On Jul 7, 2010, at 4:51 PM, Philip Zeyliger wrote:

          >
          > -----------------------------------------------------------
          > This is an automatically generated e-mail. To reply, visit:
          > http://review.hbase.org/r/281/#review320
          > -----------------------------------------------------------
          >
          >
          > The code, besides the catch (Exception e), looks fine.
          >
          > I do worry that this is a massively backwards-incompatible change. Anyone who's set up an Avro RPC server using HttpServer now needs to add a line to their code. I can only think of hacky ways around that.
          >
          Yeah, that is a concern. This is definitely an improvement though.

          >
          > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java
          > <http://review.hbase.org/r/281/#comment1391>
          >
          > I would add to the java doc that this throws AvroRuntimeException.
          >

          "@throws AvroRuntimeException if the underlying Jetty server throws any exception while starting." ?

          >
          >
          > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java
          > <http://review.hbase.org/r/281/#comment1392>
          >
          > catch (Exception e) is poor form, typically. It's usually better to explicitly catch the type of exception that Jetty's start throws.
          >

          True, but Jetty's start() signature is:

          void org.mortbay.component.AbstractLifeCycle.start() throws Exception

          So my only caution with this patch is the backwards-incompatible change. Luckily it is not subtle. I'm fine with the change but am not a major user of HttpServer. This does likely imply a change for other Servers down the road too.

          > On 2010-07-07 16:51:01, Philip Zeyliger wrote:
          > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java, line 65
          > > <http://review.hbase.org/r/281/diff/1/?file=2195#file2195line65>
          > >
          > > catch (Exception e) is poor form, typically. It's usually better to explicitly catch the type of exception that Jetty's start throws.

          Jetty's start() throws Exception: http://jetty.codehaus.org/jetty/jetty-6/apidocs/org/mortbay/component/LifeCycle.html#start%28%29

          > On 2010-07-07 16:51:01, Philip Zeyliger wrote:
          > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java, line 60
          > > <http://review.hbase.org/r/281/diff/1/?file=2195#file2195line60>
          > >
          > > I would add to the java doc that this throws AvroRuntimeException.

          Will do.

          Show
          Philip Zeyliger added a comment - From reviewboard: Jeff Hammerbacher: > On 2010-07-07 16:51:01, Philip Zeyliger wrote: > > The code, besides the catch (Exception e), looks fine. > > > > I do worry that this is a massively backwards-incompatible change. Anyone who's set up an Avro RPC server using HttpServer now needs to add a line to their code. I can only think of hacky ways around that. Well, considering that Avro RPC is under active development, I think now is the time to make massively breaking changes. The discussion on this ticket seems to indicate that the changes are for the better, so I vote for making them now while we still can. To that end, see also https://issues.apache.org/jira/browse/AVRO-594 Scott Carey: ReviewBoard is awesome. I'll need to figure out how to use it sometime. Comments inline: On Jul 7, 2010, at 4:51 PM, Philip Zeyliger wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/281/#review320 > ----------------------------------------------------------- > > > The code, besides the catch (Exception e), looks fine. > > I do worry that this is a massively backwards-incompatible change. Anyone who's set up an Avro RPC server using HttpServer now needs to add a line to their code. I can only think of hacky ways around that. > Yeah, that is a concern. This is definitely an improvement though. > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java > < http://review.hbase.org/r/281/#comment1391 > > > I would add to the java doc that this throws AvroRuntimeException. > "@throws AvroRuntimeException if the underlying Jetty server throws any exception while starting." ? > > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java > < http://review.hbase.org/r/281/#comment1392 > > > catch (Exception e) is poor form, typically. It's usually better to explicitly catch the type of exception that Jetty's start throws. > True, but Jetty's start() signature is: void org.mortbay.component.AbstractLifeCycle.start() throws Exception So my only caution with this patch is the backwards-incompatible change. Luckily it is not subtle. I'm fine with the change but am not a major user of HttpServer. This does likely imply a change for other Servers down the road too. > On 2010-07-07 16:51:01, Philip Zeyliger wrote: > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java, line 65 > > < http://review.hbase.org/r/281/diff/1/?file=2195#file2195line65 > > > > > catch (Exception e) is poor form, typically. It's usually better to explicitly catch the type of exception that Jetty's start throws. Jetty's start() throws Exception: http://jetty.codehaus.org/jetty/jetty-6/apidocs/org/mortbay/component/LifeCycle.html#start%28%29 > On 2010-07-07 16:51:01, Philip Zeyliger wrote: > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java, line 60 > > < http://review.hbase.org/r/281/diff/1/?file=2195#file2195line60 > > > > > I would add to the java doc that this throws AvroRuntimeException. Will do.
          Hide
          Jeff Hammerbacher added a comment -

          Hey Doug,

          All of the comments above have been addressed. I believe this patch is ready to go in.

          Thanks,
          Jeff

          Show
          Jeff Hammerbacher added a comment - Hey Doug, All of the comments above have been addressed. I believe this patch is ready to go in. Thanks, Jeff
          Hide
          Doug Cutting added a comment -

          +1 this looks good to me and passes tests.

          Only minor nit is that I'd replace the javadoc comment "Prepares a server to be started on the named port." with "Constructs a server that will run on the named port."

          Show
          Doug Cutting added a comment - +1 this looks good to me and passes tests. Only minor nit is that I'd replace the javadoc comment "Prepares a server to be started on the named port." with "Constructs a server that will run on the named port."
          Hide
          Doug Cutting added a comment -

          Here's a version that:

          • changes the javadoc as suggested above
          • updates SocketServer, NettyServer & DatagramServer too, plus their tests
          Show
          Doug Cutting added a comment - Here's a version that: changes the javadoc as suggested above updates SocketServer, NettyServer & DatagramServer too, plus their tests
          Hide
          Jeff Hammerbacher added a comment -

          Looks good. I haven't tested the new changes, but they seem reasonable via eyeballed code review. If the tests pass, go for it.

          Show
          Jeff Hammerbacher added a comment - Looks good. I haven't tested the new changes, but they seem reasonable via eyeballed code review. If the tests pass, go for it.
          Hide
          Scott Carey added a comment -

          +1 looks good to me.

          Show
          Scott Carey added a comment - +1 looks good to me.

            People

            • Assignee:
              Jeff Hammerbacher
              Reporter:
              Jeff Hammerbacher
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development