Apache Whirr (retired)
  1. Apache Whirr (retired)
  2. WHIRR-315

Temporary override Providers#withIds until jclouds beta-10 is out

    Details

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

      Description

      Provider#withIds is expecting a provider specific ID. This problem is fixed in jclouds-beta-10. This issue adds a temporary override. computerService.destroyNode works as expected.

      1. WHIRR-315.patch
        4 kB
        Andrei Savu
      2. WHIRR-315.patch
        3 kB
        Andrei Savu
      3. WHIRR-315.patch
        5 kB
        Andrei Savu

        Activity

        Hide
        Andrei Savu added a comment -

        I've just committed this.

        Show
        Andrei Savu added a comment - I've just committed this.
        Hide
        Andrei Savu added a comment -

        Updated issue title and description to match the patch.

        Show
        Andrei Savu added a comment - Updated issue title and description to match the patch.
        Hide
        Andrei Savu added a comment -

        Updated the patch as Adrian suggested. I'm going to commit this now.

        Show
        Andrei Savu added a comment - Updated the patch as Adrian suggested. I'm going to commit this now.
        Hide
        Adrian Cole (Inactive) added a comment -

        not a problem

        Show
        Adrian Cole (Inactive) added a comment - not a problem
        Hide
        Andrei Savu added a comment -

        Ok. I will do that. Adrian, thanks for taking a look at this.

        Show
        Andrei Savu added a comment - Ok. I will do that. Adrian, thanks for taking a look at this.
        Hide
        Adrian Cole (Inactive) added a comment -

        I think changing the public cli interface to workaround a trivial bug is a bad idea. Why don't we just paste in a correct predicate instead of changing the public interfaces to whirr? If we change the public interface to whirr, it will establish incorrect behaviour which is hard to have users change back/forth from.

        The predicate that was failing can be pasted into whirr somewhere and deprecate saying that this is fixed in beta-10.

        Here's an example, which is already in beta-10:

        /**

        • temporary override Providers#withIds until jclouds beta-10 is out
          */
          @Deprecated
          public static <T extends ComputeMetadata> Predicate<T> withIds(String... ids) {
          final Set<String> search = ImmutableSet.copyOf(checkNotNull(ids, "ids must be defined"));
          return new Predicate<T>() {
          @Override
          public boolean apply(T nodeMetadata) { return search.contains(nodeMetadata.getId()); }

        @Override
        public String toString()

        { return "withIds(" + search + ")"; }

        };
        }

        Show
        Adrian Cole (Inactive) added a comment - I think changing the public cli interface to workaround a trivial bug is a bad idea. Why don't we just paste in a correct predicate instead of changing the public interfaces to whirr? If we change the public interface to whirr, it will establish incorrect behaviour which is hard to have users change back/forth from. The predicate that was failing can be pasted into whirr somewhere and deprecate saying that this is fixed in beta-10. Here's an example, which is already in beta-10: /** temporary override Providers#withIds until jclouds beta-10 is out */ @Deprecated public static <T extends ComputeMetadata> Predicate<T> withIds(String... ids) { final Set<String> search = ImmutableSet.copyOf(checkNotNull(ids, "ids must be defined")); return new Predicate<T>() { @Override public boolean apply(T nodeMetadata) { return search.contains(nodeMetadata.getId()); } @Override public String toString() { return "withIds(" + search + ")"; } }; }
        Hide
        Andrei Savu added a comment -

        Should we open an issue to jclouds to discuss NodePredicates.withIds or this is the expected behavior and we can move forward with this patch?

        Show
        Andrei Savu added a comment - Should we open an issue to jclouds to discuss NodePredicates.withIds or this is the expected behavior and we can move forward with this patch?
        Hide
        Andrei Savu added a comment -

        I've updated the patch. I was wrong, computeService.destroyNode works as expected.

        I'm still having one issue with NodePredicates.withIds. I took a look at the implementation and it uses nodeMetadata.getProviderId() which is not equal to nodeMetadata.getId().

        Is this the expected behavior? What's the rational?

        Show
        Andrei Savu added a comment - I've updated the patch. I was wrong, computeService.destroyNode works as expected. I'm still having one issue with NodePredicates.withIds . I took a look at the implementation and it uses nodeMetadata.getProviderId() which is not equal to nodeMetadata.getId() . Is this the expected behavior? What's the rational?
        Hide
        Andrei Savu added a comment -

        I will check again. Maybe I'm doing something wrong.

        Show
        Andrei Savu added a comment - I will check again. Maybe I'm doing something wrong.
        Hide
        Adrian Cole (Inactive) added a comment -

        Have you debugged to see the value being passed to compute.destroyNode(id) ?

        The code for the jclouds ec2 impl (EC2DestroyNodeStrategy) is the following, so you can imagine why this patch makes me scratch my head:

        public NodeMetadata destroyNode(String id) {
        String[] parts = AWSUtils.parseHandle(id);
        String region = parts[0];
        String instanceId = parts[1];
        -snip-

        Show
        Adrian Cole (Inactive) added a comment - Have you debugged to see the value being passed to compute.destroyNode(id) ? The code for the jclouds ec2 impl (EC2DestroyNodeStrategy) is the following, so you can imagine why this patch makes me scratch my head: public NodeMetadata destroyNode(String id) { String[] parts = AWSUtils.parseHandle(id); String region = parts [0] ; String instanceId = parts [1] ; - snip -
        Hide
        Adrian Cole (Inactive) added a comment -

        This sounds very surprising. I'll have a look.

        Show
        Adrian Cole (Inactive) added a comment - This sounds very surprising. I'll have a look.
        Hide
        Andrei Savu added a comment -

        Attached a patch that replaces string.split with AWSUtils.parseHandle.

        Show
        Andrei Savu added a comment - Attached a patch that replaces string.split with AWSUtils.parseHandle.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development