[ 
https://issues.apache.org/jira/browse/CURATOR-457?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Aleksander Melnichnikov updated CURATOR-457:
--------------------------------------------
    Description: 
Right now paths for services in service discovery hardcoded to pattern: 
*/\{basePath} /\{serviceName}/\{instanceId}.* And that's impossible to 
configure paths this way:  */\{basePathPart1} 
/\{serviceName}/\{basePathPart2}/\{instanceId}*

For example:
 If basePath = */base1/base2/base3*, serviceName = *serviceName*, instanceId - 
*serviceId*
 then path will be 
*/**base1**/**base2**/**base3**/**serviceName**/**serviceId*, that can't be 
configured like this: */**base1**/**base2/serviceName**/**base3**/**serviceId.*

Because of strong encapsulation in ServiceDiscoveryImpl class (package private 
method modifiers)
{code:java}
@VisibleForTesting
String pathForName(String name) 
{ 
    return ZKPaths.makePath(basePath, name); 
}
@VisibleForTesting 
String pathForInstance(String name, String id)
{ 
   return ZKPaths.makePath(pathForName(name), id);
}
{code}
So, can we rethink visibility modifiers or add some extendable entity which 
would be responsible for constructing paths? 

For example:

Create interface
{code:java}
/**
 * A constructor which constructs path to services for service discovering
 */
public interface DiscoveryPathConstructor
{
    /**
     * Return the path where all service names registered
     * @return basePath
     */
    String getBasePath();

    /**
     * Return the path where all instances of service registered
     * @param name service name
     * @return path to service instances
     */
    String getPathForInstances(String name);

    /**
     * Return the path where concrete instance registered
     * @param name service name
     * @param id concrete instance id
     * @return path to concrete instance
     */
    String getPathForInstance(String name, String id);
}
{code}

And make a constructor and field in ServiceDiscoveryImpl:

{code:java}
    private final DiscoveryPathConstructor pathConstructor;
  /**
     * @param client the client
     * @param pathConstructor constructor for instances path
     * @param serializer serializer for instances (e.g. {@link 
JsonInstanceSerializer})
     * @param thisInstance instance that represents the service that is 
running. The instance will get auto-registered
     * @param watchInstances if true, watches for changes to locally registered 
instances
     */
    public ServiceDiscoveryImpl(CuratorFramework client, 
DiscoveryPathConstructor pathConstructor, InstanceSerializer<T> serializer,
                                ServiceInstance<T> thisInstance, boolean 
watchInstances)
    {
        this.watchInstances = watchInstances;
        this.client = Preconditions.checkNotNull(client, "client cannot be 
null");
        this.serializer = Preconditions.checkNotNull(serializer, "serializer 
cannot be null");
        this.pathConstructor = Preconditions.checkNotNull(pathConstructor, 
"pathConstructor cannot be null");
        if ( thisInstance != null )
        {
            Entry<T> entry = new Entry<T>(thisInstance);
            entry.cache = makeNodeCache(thisInstance);
            services.put(thisInstance.getId(), entry);
        }
    }
{code}

You can view commit in my fork of framework, if that's ok - I can make a pull 
request. 
https://github.com/Me1kaa/curator/commit/6e6a34db7d71a84e3a53d284fc94e1fe2e3c10d8


  was:
Right now paths for services in service discovery hardcoded to pattern: 
*/\{basePath} /\{serviceName}/\{instanceId}.* And that's impossible to 
configure paths this way:  */\{basePathPart1} 
/\{serviceName}/{basePathPart2}/\{instanceId}**

For example:
 If basePath = */base1/base2/base3*, serviceName = *serviceName*, instanceId - 
*serviceId*
 then path will be 
*/**base1**/**base2**/**base3**/**serviceName**/**serviceId*, that can't be 
configured like this: */**base1**/**base2/serviceName**/**base3**/**serviceId.*

Because of strong encapsulation in ServiceDiscoveryImpl class (package private 
method modifiers)
{code:java}
@VisibleForTesting
String pathForName(String name) 
{ 
    return ZKPaths.makePath(basePath, name); 
}
@VisibleForTesting 
String pathForInstance(String name, String id)
{ 
   return ZKPaths.makePath(pathForName(name), id);
}
{code}
So, can we rethink visibility modifiers or add some extendable entity which 
would be responsible for constructing paths? 

For example:

Create interface
{code:java}
/**
 * A constructor which constructs path to services for service discovering
 */
public interface DiscoveryPathConstructor
{
    /**
     * Return the path where all service names registered
     * @return basePath
     */
    String getBasePath();

    /**
     * Return the path where all instances of service registered
     * @param name service name
     * @return path to service instances
     */
    String getPathForInstances(String name);

    /**
     * Return the path where concrete instance registered
     * @param name service name
     * @param id concrete instance id
     * @return path to concrete instance
     */
    String getPathForInstance(String name, String id);
}
{code}

And make a constructor and field in ServiceDiscoveryImpl:

{code:java}
    private final DiscoveryPathConstructor pathConstructor;
  /**
     * @param client the client
     * @param pathConstructor constructor for instances path
     * @param serializer serializer for instances (e.g. {@link 
JsonInstanceSerializer})
     * @param thisInstance instance that represents the service that is 
running. The instance will get auto-registered
     * @param watchInstances if true, watches for changes to locally registered 
instances
     */
    public ServiceDiscoveryImpl(CuratorFramework client, 
DiscoveryPathConstructor pathConstructor, InstanceSerializer<T> serializer,
                                ServiceInstance<T> thisInstance, boolean 
watchInstances)
    {
        this.watchInstances = watchInstances;
        this.client = Preconditions.checkNotNull(client, "client cannot be 
null");
        this.serializer = Preconditions.checkNotNull(serializer, "serializer 
cannot be null");
        this.pathConstructor = Preconditions.checkNotNull(pathConstructor, 
"pathConstructor cannot be null");
        if ( thisInstance != null )
        {
            Entry<T> entry = new Entry<T>(thisInstance);
            entry.cache = makeNodeCache(thisInstance);
            services.put(thisInstance.getId(), entry);
        }
    }
{code}

You can view commit in my fork of framework, if that's ok - I can make a pull 
request. 
https://github.com/Me1kaa/curator/commit/6e6a34db7d71a84e3a53d284fc94e1fe2e3c10d8



> Configuring service discovery paths
> -----------------------------------
>
>                 Key: CURATOR-457
>                 URL: https://issues.apache.org/jira/browse/CURATOR-457
>             Project: Apache Curator
>          Issue Type: Improvement
>            Reporter: Aleksander Melnichnikov
>            Assignee: Jordan Zimmerman
>            Priority: Minor
>
> Right now paths for services in service discovery hardcoded to pattern: 
> */\{basePath} /\{serviceName}/\{instanceId}.* And that's impossible to 
> configure paths this way:  */\{basePathPart1} 
> /\{serviceName}/\{basePathPart2}/\{instanceId}*
> For example:
>  If basePath = */base1/base2/base3*, serviceName = *serviceName*, instanceId 
> - *serviceId*
>  then path will be 
> */**base1**/**base2**/**base3**/**serviceName**/**serviceId*, that can't be 
> configured like this: 
> */**base1**/**base2/serviceName**/**base3**/**serviceId.*
> Because of strong encapsulation in ServiceDiscoveryImpl class (package 
> private method modifiers)
> {code:java}
> @VisibleForTesting
> String pathForName(String name) 
> { 
>     return ZKPaths.makePath(basePath, name); 
> }
> @VisibleForTesting 
> String pathForInstance(String name, String id)
> { 
>    return ZKPaths.makePath(pathForName(name), id);
> }
> {code}
> So, can we rethink visibility modifiers or add some extendable entity which 
> would be responsible for constructing paths? 
> For example:
> Create interface
> {code:java}
> /**
>  * A constructor which constructs path to services for service discovering
>  */
> public interface DiscoveryPathConstructor
> {
>     /**
>      * Return the path where all service names registered
>      * @return basePath
>      */
>     String getBasePath();
>     /**
>      * Return the path where all instances of service registered
>      * @param name service name
>      * @return path to service instances
>      */
>     String getPathForInstances(String name);
>     /**
>      * Return the path where concrete instance registered
>      * @param name service name
>      * @param id concrete instance id
>      * @return path to concrete instance
>      */
>     String getPathForInstance(String name, String id);
> }
> {code}
> And make a constructor and field in ServiceDiscoveryImpl:
> {code:java}
>     private final DiscoveryPathConstructor pathConstructor;
>   /**
>      * @param client the client
>      * @param pathConstructor constructor for instances path
>      * @param serializer serializer for instances (e.g. {@link 
> JsonInstanceSerializer})
>      * @param thisInstance instance that represents the service that is 
> running. The instance will get auto-registered
>      * @param watchInstances if true, watches for changes to locally 
> registered instances
>      */
>     public ServiceDiscoveryImpl(CuratorFramework client, 
> DiscoveryPathConstructor pathConstructor, InstanceSerializer<T> serializer,
>                                 ServiceInstance<T> thisInstance, boolean 
> watchInstances)
>     {
>         this.watchInstances = watchInstances;
>         this.client = Preconditions.checkNotNull(client, "client cannot be 
> null");
>         this.serializer = Preconditions.checkNotNull(serializer, "serializer 
> cannot be null");
>         this.pathConstructor = Preconditions.checkNotNull(pathConstructor, 
> "pathConstructor cannot be null");
>         if ( thisInstance != null )
>         {
>             Entry<T> entry = new Entry<T>(thisInstance);
>             entry.cache = makeNodeCache(thisInstance);
>             services.put(thisInstance.getId(), entry);
>         }
>     }
> {code}
> You can view commit in my fork of framework, if that's ok - I can make a pull 
> request. 
> https://github.com/Me1kaa/curator/commit/6e6a34db7d71a84e3a53d284fc94e1fe2e3c10d8



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to