Whirr
  1. Whirr
  2. WHIRR-61

make more efficient use of ComputeServiceContext

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.5.0
    • Component/s: None
    • Labels:
      None

      Description

      The ComputeService object is thread-safe, and better off shared inside the Service object. Regardless, it must be closed in order to release threadpools.

      1. WHIRR-61.patch
        67 kB
        Tom White
      2. WHIRR-61.patch
        66 kB
        Adrian Cole
      3. WHIRR-61.patch
        20 kB
        Andrei Savu
      4. WHIRR-61.patch
        20 kB
        Andrei Savu
      5. WHIRR-61.patch
        10 kB
        Andrei Savu
      6. WHIRR-61.patch
        5 kB
        Andrei Savu

        Issue Links

          Activity

          Adrian Cole created issue -
          Hide
          Andrei Savu added a comment -

          We could change the behavior of ComputeServiceContextBuilder so that it will create only one instance per ClusterSpec.

          Show
          Andrei Savu added a comment - We could change the behavior of ComputeServiceContextBuilder so that it will create only one instance per ClusterSpec.
          Hide
          Tom White added a comment -

          Sounds good. I seem to remember that creating a ComputeServiceContext is a fairly expensive operation, so implementing this would make things a bit faster.

          Show
          Tom White added a comment - Sounds good. I seem to remember that creating a ComputeServiceContext is a fairly expensive operation, so implementing this would make things a bit faster.
          Hide
          Andrei Savu added a comment -

          Adrian any ideas how expensive (time) is this compared to starting and configuring a cluster instance? I'm not sure if this optimization is really needed now. Are you guys thinking about expensive in terms of memory?

          Show
          Andrei Savu added a comment - Adrian any ideas how expensive (time) is this compared to starting and configuring a cluster instance? I'm not sure if this optimization is really needed now. Are you guys thinking about expensive in terms of memory?
          Hide
          Adrian Cole added a comment - - edited

          @andrei the largest startup time is parsing images, locations, and hardware profiles in the cloud. This is a function of the response time of the rest requests from the server, which vary dramatically from 150ms -> 30 seconds (image fetch in amazon). jclouds optimizes this as much as possible. blobstore context startup only saves a couple seconds or less, as the only thing parsed there are locations (ex. getBucketLocation in s3).

          Show
          Adrian Cole added a comment - - edited @andrei the largest startup time is parsing images, locations, and hardware profiles in the cloud. This is a function of the response time of the rest requests from the server, which vary dramatically from 150ms -> 30 seconds (image fetch in amazon). jclouds optimizes this as much as possible. blobstore context startup only saves a couple seconds or less, as the only thing parsed there are locations (ex. getBucketLocation in s3).
          Hide
          Andrei Savu added a comment -

          Ok. I will provide a patch later today and do some performance tests.

          Show
          Andrei Savu added a comment - Ok. I will provide a patch later today and do some performance tests.
          Hide
          Andrei Savu added a comment -

          I've done some (dummy) benchmarking on this patch by running the HBase integration tests. It takes 7m20.738s with cache versus 7m46.069s without using the cache (highly unreliable results).

          The current patch is unsafe. We should either make a ClusterSpec copy or make that class immutable (harder but I believe it's better on the long term). What do you think?

          Show
          Andrei Savu added a comment - I've done some (dummy) benchmarking on this patch by running the HBase integration tests. It takes 7m20.738s with cache versus 7m46.069s without using the cache (highly unreliable results). The current patch is unsafe. We should either make a ClusterSpec copy or make that class immutable (harder but I believe it's better on the long term). What do you think?
          Andrei Savu made changes -
          Field Original Value New Value
          Attachment WHIRR-61.patch [ 12478046 ]
          Hide
          Andrei Savu added a comment -

          Updated patch to create copies from ClusterSpec when used as a cache key.

          I've also done some searching and it seems like we are only updating a ClusterSpec instance in the test code.

          Show
          Andrei Savu added a comment - Updated patch to create copies from ClusterSpec when used as a cache key. I've also done some searching and it seems like we are only updating a ClusterSpec instance in the test code.
          Andrei Savu made changes -
          Attachment WHIRR-61.patch [ 12478166 ]
          Andrei Savu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Assignee Andrei Savu [ savu.andrei ]
          Fix Version/s 0.5.0 [ 12316248 ]
          Hide
          Andrei Savu added a comment -

          What do you think about this one? Does the performance improvement justifies the increase in code complexity?

          Show
          Andrei Savu added a comment - What do you think about this one? Does the performance improvement justifies the increase in code complexity?
          Hide
          Andrei Savu added a comment -

          It seems like the patch is broken. I will resubmit later.

          Show
          Andrei Savu added a comment - It seems like the patch is broken. I will resubmit later.
          Andrei Savu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Tom White added a comment -

          This looks reasonable to me (but it's be great if Adrian could take a look too). I agree that it would be better to make ClusterSpec immutable in the future (by using the builder pattern).

          Show
          Tom White added a comment - This looks reasonable to me (but it's be great if Adrian could take a look too). I agree that it would be better to make ClusterSpec immutable in the future (by using the builder pattern).
          Hide
          Andrei Savu added a comment -

          Fixed some mutability related bugs. Adrian let me know what you think.

          Show
          Andrei Savu added a comment - Fixed some mutability related bugs. Adrian let me know what you think.
          Andrei Savu made changes -
          Attachment WHIRR-61.patch [ 12478422 ]
          Andrei Savu made changes -
          Link This issue is related to WHIRR-303 [ WHIRR-303 ]
          Andrei Savu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Andrei Savu added a comment -

          Any feedback is highly appreciated

          Show
          Andrei Savu added a comment - Any feedback is highly appreciated
          Hide
          Adrian Cole added a comment -

          Good start. the following suggestions are mainly just cleanups

          I'd probably use MapMaker as it will eliminate the checkNull race condition on making a new blobstore or computeservice. you can also avoid doing a .copy() on the ClusterSpec by keying your persistence map on an object summarizing the unique inputs to createContext. Typically, this is provider, identity, credential, endpoint (part of properties).

          Show
          Adrian Cole added a comment - Good start. the following suggestions are mainly just cleanups I'd probably use MapMaker as it will eliminate the checkNull race condition on making a new blobstore or computeservice. you can also avoid doing a .copy() on the ClusterSpec by keying your persistence map on an object summarizing the unique inputs to createContext. Typically, this is provider, identity, credential, endpoint (part of properties).
          Hide
          Andrei Savu added a comment -

          Added a class for summarizing the unique inputs to createContext. I will do more cleanup work in WHIRR-303. Good to go?

          Show
          Andrei Savu added a comment - Added a class for summarizing the unique inputs to createContext. I will do more cleanup work in WHIRR-303 . Good to go?
          Andrei Savu made changes -
          Attachment WHIRR-61.patch [ 12478949 ]
          Hide
          Adrian Cole added a comment - - edited

          here's a patch that decouples all the classes from the compute service cache, and makes a nice threadsafe one.

          Side note:

          I've fixed a few broken windows (ex. unused imports & fields, missing generic type params, etc) and included these in the same patch intentionally.

          While it is a minor pain to glance at these small fixes while also looking at the core changes, I strongly believe it is for the better. We should make it easier to fix broken windows then to break them. Jailing window-fixing into separate jiras will surely demotivate those of us who like keep the project clean. Being flexible about allowing trivial fixes in patches will encourage us.

          Show
          Adrian Cole added a comment - - edited here's a patch that decouples all the classes from the compute service cache, and makes a nice threadsafe one. Side note: I've fixed a few broken windows (ex. unused imports & fields, missing generic type params, etc) and included these in the same patch intentionally. While it is a minor pain to glance at these small fixes while also looking at the core changes, I strongly believe it is for the better. We should make it easier to fix broken windows then to break them. Jailing window-fixing into separate jiras will surely demotivate those of us who like keep the project clean. Being flexible about allowing trivial fixes in patches will encourage us.
          Adrian Cole made changes -
          Attachment WHIRR-61.patch [ 12479050 ]
          Hide
          Adrian Cole added a comment -

          recent patch tested on elastichosts aws-ec2; cassandra aws-ec2, cloudservers-us

          Show
          Adrian Cole added a comment - recent patch tested on elastichosts aws-ec2; cassandra aws-ec2, cloudservers-us
          Hide
          Tom White added a comment -

          > I've fixed a few broken windows (ex. unused imports & fields, missing generic type params, etc) and included these in the same patch intentionally.

          Thanks for fixing these, Adrian. In general, developers should run mvn checkstyle:checkstyle and mvn apache-rat:check before patch submission, and committers should do the same before commit.

          Show
          Tom White added a comment - > I've fixed a few broken windows (ex. unused imports & fields, missing generic type params, etc) and included these in the same patch intentionally. Thanks for fixing these, Adrian. In general, developers should run mvn checkstyle:checkstyle and mvn apache-rat:check before patch submission, and committers should do the same before commit.
          Hide
          Andrei Savu added a comment -

          +1 (this is the first time I see the singleton enum pattern)

          Tom, would it be possible to use an Apache CI server to run unit tests, checkstyle and apache-rat when a new patch is submitted?

          Show
          Andrei Savu added a comment - +1 (this is the first time I see the singleton enum pattern) Tom, would it be possible to use an Apache CI server to run unit tests, checkstyle and apache-rat when a new patch is submitted?
          Hide
          Tom White added a comment -

          I had some problems getting the Apache CI server to run checkstyle and RAT for Whirr, but we should look into this again. See WHIRR-307.

          Getting it to run on check in is straightforward, patch submission is more difficult (Hadoop does it, but I'm not sure how). Let's start with check in to start with.

          Show
          Tom White added a comment - I had some problems getting the Apache CI server to run checkstyle and RAT for Whirr, but we should look into this again. See WHIRR-307 . Getting it to run on check in is straightforward, patch submission is more difficult (Hadoop does it, but I'm not sure how). Let's start with check in to start with.
          Hide
          Tom White added a comment -

          +1

          I've updated the patch with a few minor changes to get checkstyle and RAT to pass. I'm going to commit this in a minute.

          Show
          Tom White added a comment - +1 I've updated the patch with a few minor changes to get checkstyle and RAT to pass. I'm going to commit this in a minute.
          Tom White made changes -
          Attachment WHIRR-61.patch [ 12479248 ]
          Hide
          Tom White added a comment -

          I've just committed this. Thanks, Adrian and Andrei!

          Show
          Tom White added a comment - I've just committed this. Thanks, Adrian and Andrei!
          Tom White made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Adrian Cole added a comment -

          thanks, tom!

          Show
          Adrian Cole added a comment - thanks, tom!

            People

            • Assignee:
              Andrei Savu
              Reporter:
              Adrian Cole
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development