Details

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

      Description

      Discoverables store only a name and an InetSocketAddress. It would help extensibility to allow an arbitrary payload of bytes.

        Issue Links

          Activity

          Hide
          stevel@apache.org Steve Loughran added a comment -

          It'd be convenient if you used the same format as the YARN registry, as it'd make running in there trivial. See YARN-2678 for the latest (final) record structure.

          There's a couple of consequences of arbitrary byte arrays

          1. they don't let other apps examine or act on them.
          2. it's dangerously easy to keep putting more and more stuff in there, until your ZK cluster gets overloaded. The default 1MB/znode limit is designed to restrict this, and with a low rate of change should be manageable —until you get to a large cluster with many apps
          Show
          stevel@apache.org Steve Loughran added a comment - It'd be convenient if you used the same format as the YARN registry, as it'd make running in there trivial. See YARN-2678 for the latest (final) record structure. There's a couple of consequences of arbitrary byte arrays they don't let other apps examine or act on them. it's dangerously easy to keep putting more and more stuff in there, until your ZK cluster gets overloaded. The default 1MB/znode limit is designed to restrict this, and with a low rate of change should be manageable —until you get to a large cluster with many apps
          Hide
          ParthGandhi Parth Gandhi added a comment -

          I'm not entirely sure what you mean.

          Seems like the difference is that the JIRA says that multiple objects are stored in the JSON blob, while here we have individual ephemeral nodes in ZK for each object (i.e discoverable). Also, is there validation of the JSON blob that gets stored, and does it happen server side? In that case, are you restricted in terms of what you can store?

          While I understand your concerns with having arbitrary byte arrays, I don't think I fully understand what you're alluding to.

          Show
          ParthGandhi Parth Gandhi added a comment - I'm not entirely sure what you mean. Seems like the difference is that the JIRA says that multiple objects are stored in the JSON blob, while here we have individual ephemeral nodes in ZK for each object (i.e discoverable). Also, is there validation of the JSON blob that gets stored, and does it happen server side? In that case, are you restricted in terms of what you can store? While I understand your concerns with having arbitrary byte arrays, I don't think I fully understand what you're alluding to.
          Hide
          stevel@apache.org Steve Loughran added a comment -
          1. we use one node per service, with each endpoint listed in the internal/exported lists (idea taken from Helix)
          2. this gives us a view of a service as a whole, being able to go from one of its exported services to an adjacent one
          3. individual components can also register themselves (JMX ports, etc)
          4. Not using ephemeral nodes as it is (a) brittle against connection loss and (b) won't work over our planned REST API. We have some tie-in to yarn container, attempt and app lifespans, with the YARN RM triggering cleanup operations.
          5. you can stick extra k:v pairs into the JSON, they just get ignored. Recommendation is to prefix them, to get attributes like twill:auth and twill:owner
          Show
          stevel@apache.org Steve Loughran added a comment - we use one node per service, with each endpoint listed in the internal/exported lists (idea taken from Helix) this gives us a view of a service as a whole, being able to go from one of its exported services to an adjacent one individual components can also register themselves (JMX ports, etc) Not using ephemeral nodes as it is (a) brittle against connection loss and (b) won't work over our planned REST API. We have some tie-in to yarn container, attempt and app lifespans, with the YARN RM triggering cleanup operations. you can stick extra k:v pairs into the JSON, they just get ignored. Recommendation is to prefix them, to get attributes like twill:auth and twill:owner
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on the pull request:

          https://github.com/apache/incubator-twill/pull/18#issuecomment-122130770

          Any chance to rebase and update it @ParthGandhi ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on the pull request: https://github.com/apache/incubator-twill/pull/18#issuecomment-122130770 Any chance to rebase and update it @ParthGandhi ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user anwar6953 commented on the pull request:

          https://github.com/apache/incubator-twill/pull/18#issuecomment-127719363

          Hey, @ParthGandhi can you close this PR for now?
          If we decide to take this to completion later, we can simply start off your branch then.

          Show
          githubbot ASF GitHub Bot added a comment - Github user anwar6953 commented on the pull request: https://github.com/apache/incubator-twill/pull/18#issuecomment-127719363 Hey, @ParthGandhi can you close this PR for now? If we decide to take this to completion later, we can simply start off your branch then.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ParthGandhi commented on the pull request:

          https://github.com/apache/incubator-twill/pull/18#issuecomment-127726739

          Sorry guys, I've had no time to finish this up.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ParthGandhi commented on the pull request: https://github.com/apache/incubator-twill/pull/18#issuecomment-127726739 Sorry guys, I've had no time to finish this up.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ParthGandhi closed the pull request at:

          https://github.com/apache/incubator-twill/pull/18

          Show
          githubbot ASF GitHub Bot added a comment - Github user ParthGandhi closed the pull request at: https://github.com/apache/incubator-twill/pull/18
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user gokulavasan opened a pull request:

          https://github.com/apache/twill/pull/10

          TWILL-107 Add payload support for Discoverable

          JIRA : https://issues.apache.org/jira/browse/TWILL-107

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/gokulavasan/twill feature/twill-107

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/twill/pull/10.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #10


          commit 5f82fe12e12a6cf42ad7ac43cde4e9b3fb0fec53
          Author: Gokul Gunasekaran <gokul@cask.co>
          Date: 2016-09-09T01:12:43Z

          TWILL-107 Add payload support for Discoverable

          Signed-off-by: Gokul Gunasekaran <gokul@cask.co>


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user gokulavasan opened a pull request: https://github.com/apache/twill/pull/10 TWILL-107 Add payload support for Discoverable JIRA : https://issues.apache.org/jira/browse/TWILL-107 You can merge this pull request into a Git repository by running: $ git pull https://github.com/gokulavasan/twill feature/twill-107 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/twill/pull/10.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #10 commit 5f82fe12e12a6cf42ad7ac43cde4e9b3fb0fec53 Author: Gokul Gunasekaran <gokul@cask.co> Date: 2016-09-09T01:12:43Z TWILL-107 Add payload support for Discoverable Signed-off-by: Gokul Gunasekaran <gokul@cask.co>
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/10#discussion_r78250500

          — Diff: twill-discovery-api/src/main/java/org/apache/twill/discovery/Discoverable.java —
          @@ -19,19 +19,67 @@
          package org.apache.twill.discovery;

          import java.net.InetSocketAddress;
          +import java.util.Arrays;

          /**

          • Discoverable defines the attributes of service to be discovered.
            */
            -public interface Discoverable {
            +public class Discoverable {
            + private final String name;
            + private final InetSocketAddress address;
            + private final byte[] payload;

          + public Discoverable(String name, InetSocketAddress address, byte[] payload) {
          — End diff –

          Should also have a constructor that doesn't take `payload`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/10#discussion_r78250500 — Diff: twill-discovery-api/src/main/java/org/apache/twill/discovery/Discoverable.java — @@ -19,19 +19,67 @@ package org.apache.twill.discovery; import java.net.InetSocketAddress; +import java.util.Arrays; /** Discoverable defines the attributes of service to be discovered. */ -public interface Discoverable { +public class Discoverable { + private final String name; + private final InetSocketAddress address; + private final byte[] payload; + public Discoverable(String name, InetSocketAddress address, byte[] payload) { — End diff – Should also have a constructor that doesn't take `payload`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/10#discussion_r78250557

          — Diff: twill-discovery-api/src/main/java/org/apache/twill/discovery/Discoverable.java —
          @@ -19,19 +19,67 @@
          package org.apache.twill.discovery;

          import java.net.InetSocketAddress;
          +import java.util.Arrays;

          /**

          • Discoverable defines the attributes of service to be discovered.
            */
            -public interface Discoverable {
            +public class Discoverable {
            + private final String name;
            + private final InetSocketAddress address;
            + private final byte[] payload;

          + public Discoverable(String name, InetSocketAddress address, byte[] payload)

          { + this.name = name; + this.address = address; + this.payload = payload; + }

          /**

          • @return Name of the service
            */
          • String getName();
            + public String getName() { + return name; + }

          /**

          • @return An {@link InetSocketAddress}

            representing the host+port of the service.
            */

          • InetSocketAddress getSocketAddress();
            + public InetSocketAddress getSocketAddress() { + return address; + }

            +
            + /**
            + * @return A payload represented as a byte array
            + */
            + byte[] getPayload() {

              • End diff –

          Should this be a `public` method?

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/10#discussion_r78250557 — Diff: twill-discovery-api/src/main/java/org/apache/twill/discovery/Discoverable.java — @@ -19,19 +19,67 @@ package org.apache.twill.discovery; import java.net.InetSocketAddress; +import java.util.Arrays; /** Discoverable defines the attributes of service to be discovered. */ -public interface Discoverable { +public class Discoverable { + private final String name; + private final InetSocketAddress address; + private final byte[] payload; + public Discoverable(String name, InetSocketAddress address, byte[] payload) { + this.name = name; + this.address = address; + this.payload = payload; + } /** @return Name of the service */ String getName(); + public String getName() { + return name; + } /** @return An {@link InetSocketAddress} representing the host+port of the service. */ InetSocketAddress getSocketAddress(); + public InetSocketAddress getSocketAddress() { + return address; + } + + /** + * @return A payload represented as a byte array + */ + byte[] getPayload() { End diff – Should this be a `public` method?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/10#discussion_r78250733

          — Diff: twill-discovery-api/src/main/java/org/apache/twill/discovery/Discoverable.java —
          @@ -19,19 +19,67 @@
          package org.apache.twill.discovery;

          import java.net.InetSocketAddress;
          +import java.util.Arrays;

          /**

          • Discoverable defines the attributes of service to be discovered.
            */
            -public interface Discoverable {
            +public class Discoverable {
            + private final String name;
            + private final InetSocketAddress address;
            + private final byte[] payload;

          + public Discoverable(String name, InetSocketAddress address, byte[] payload)

          { + this.name = name; + this.address = address; + this.payload = payload; + }

          /**

          • @return Name of the service
            */
          • String getName();
            + public String getName() { + return name; + }

          /**

          • @return An {@link InetSocketAddress}

            representing the host+port of the service.
            */

          • InetSocketAddress getSocketAddress();
            + public InetSocketAddress getSocketAddress() { + return address; + }

            +
            + /**
            + * @return A payload represented as a byte array
            + */
            + byte[] getPayload()

            { + return payload; + }

            +
            + @Override
            + public String toString()

            Unknown macro: { + return "{name=" + name + ", address=" + address + ", payload=" + payload + "}"; + }

            +
            + @Override
            + public boolean equals(Object o)

            Unknown macro: { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + Discoverable other = (Discoverable) o; + + return name.equals(other.getName()) && address.equals(other.getSocketAddress()) && + Arrays.equals(payload, other.getPayload()); + }

            +
            + @Override
            + public int hashCode() {
            + int result = name.hashCode();

              • End diff –

          Use `Objects.hash(name, address, payload)` instead.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/10#discussion_r78250733 — Diff: twill-discovery-api/src/main/java/org/apache/twill/discovery/Discoverable.java — @@ -19,19 +19,67 @@ package org.apache.twill.discovery; import java.net.InetSocketAddress; +import java.util.Arrays; /** Discoverable defines the attributes of service to be discovered. */ -public interface Discoverable { +public class Discoverable { + private final String name; + private final InetSocketAddress address; + private final byte[] payload; + public Discoverable(String name, InetSocketAddress address, byte[] payload) { + this.name = name; + this.address = address; + this.payload = payload; + } /** @return Name of the service */ String getName(); + public String getName() { + return name; + } /** @return An {@link InetSocketAddress} representing the host+port of the service. */ InetSocketAddress getSocketAddress(); + public InetSocketAddress getSocketAddress() { + return address; + } + + /** + * @return A payload represented as a byte array + */ + byte[] getPayload() { + return payload; + } + + @Override + public String toString() Unknown macro: { + return "{name=" + name + ", address=" + address + ", payload=" + payload + "}"; + } + + @Override + public boolean equals(Object o) Unknown macro: { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + Discoverable other = (Discoverable) o; + + return name.equals(other.getName()) && address.equals(other.getSocketAddress()) && + Arrays.equals(payload, other.getPayload()); + } + + @Override + public int hashCode() { + int result = name.hashCode(); End diff – Use `Objects.hash(name, address, payload)` instead.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on a diff in the pull request:

          https://github.com/apache/twill/pull/10#discussion_r78250998

          — Diff: twill-discovery-core/src/main/java/org/apache/twill/discovery/DiscoverableAdapter.java —
          @@ -76,18 +78,9 @@ public Discoverable deserialize(JsonElement json, Type typeOfT,
          final String service = jsonObj.get("service").getAsString();
          String hostname = jsonObj.get("hostname").getAsString();
          int port = jsonObj.get("port").getAsInt();
          + final byte[] payload = context.deserialize(jsonObj.get("payload"), BYTE_ARRAY_TYPE);
          — End diff –

          No need to be final. Same for `service` above.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on a diff in the pull request: https://github.com/apache/twill/pull/10#discussion_r78250998 — Diff: twill-discovery-core/src/main/java/org/apache/twill/discovery/DiscoverableAdapter.java — @@ -76,18 +78,9 @@ public Discoverable deserialize(JsonElement json, Type typeOfT, final String service = jsonObj.get("service").getAsString(); String hostname = jsonObj.get("hostname").getAsString(); int port = jsonObj.get("port").getAsInt(); + final byte[] payload = context.deserialize(jsonObj.get("payload"), BYTE_ARRAY_TYPE); — End diff – No need to be final. Same for `service` above.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/10

          Just couple comments.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/10 Just couple comments.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user chtyim commented on the issue:

          https://github.com/apache/twill/pull/10

          LGTM. Please squash the commits.

          Show
          githubbot ASF GitHub Bot added a comment - Github user chtyim commented on the issue: https://github.com/apache/twill/pull/10 LGTM. Please squash the commits.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

          https://github.com/apache/twill/pull/10

          @chtyim Squashed commits. Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/twill/pull/10 @chtyim Squashed commits. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/twill/pull/10

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/twill/pull/10
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hsaputra commented on the issue:

          https://github.com/apache/twill/pull/10

          Thanks for the PR, @gokulavasan. Next time would love to have more information how you solve it in the PR description rather than just link to JIRA.

          Show
          githubbot ASF GitHub Bot added a comment - Github user hsaputra commented on the issue: https://github.com/apache/twill/pull/10 Thanks for the PR, @gokulavasan. Next time would love to have more information how you solve it in the PR description rather than just link to JIRA.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

          https://github.com/apache/twill/pull/10

          @hsaputra I have added some description to the PR. Sorry. Thank you for catching it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/twill/pull/10 @hsaputra I have added some description to the PR. Sorry. Thank you for catching it.

            People

            • Assignee:
              gokulavasan Gokul Gunasekaran
              Reporter:
              ParthGandhi Parth Gandhi
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development