Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Create a ServiceRegistry interface.
Allows service discovery systems to register more than one instance.

Automatic registration will still take place.

fixes gh-9
  • Loading branch information
spencergibb committed Nov 4, 2016
commit aa8318e64faaaf3d49617d86ba2e699aaa45025e
2 changes: 1 addition & 1 deletion spring-cloud-commons-dependencies/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<artifactId>spring-cloud-dependencies-parent</artifactId>
<groupId>org.springframework.cloud</groupId>
<version>1.1.2.RELEASE</version>
<version>1.2.2.BUILD-SNAPSHOT</version>
<relativePath/>
</parent>
<artifactId>spring-cloud-commons-dependencies</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.springframework.beans.BeansException;
import org.springframework.boot.context.embedded.EmbeddedServletContainerInitializedEvent;
import org.springframework.cloud.client.discovery.event.InstanceRegisteredEvent;
import org.springframework.cloud.client.serviceregistry.Registration;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how this is mixed up here. How about I move *Lifecycle to it's own package?

Copy link
Member Author

@spencergibb spencergibb Jul 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, DiscoveryLifecycle doesn't even make sense anymore. Maybe AutoRegistration? It's not a function of discovery at all.

import org.springframework.cloud.client.serviceregistry.ServiceRegistry;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
import org.springframework.context.ApplicationListener;
Expand All @@ -35,7 +37,7 @@
* Lifecycle methods that may be useful and common to various DiscoveryClient implementations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it still various? or is registration (the R type) intended to work only with one sort of client now? I'd probably doc the type param regardless of whether it stays a marker or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R will be specific to the impl (though it won't extend Registration).

* @author Spencer Gibb
*/
public abstract class AbstractDiscoveryLifecycle implements DiscoveryLifecycle,
public abstract class AbstractDiscoveryLifecycle<R extends Registration> implements DiscoveryLifecycle,
ApplicationContextAware, ApplicationListener<EmbeddedServletContainerInitializedEvent> {

private static final Log logger = LogFactory.getLog(AbstractDiscoveryLifecycle.class);
Expand All @@ -52,6 +54,12 @@ public abstract class AbstractDiscoveryLifecycle implements DiscoveryLifecycle,

private AtomicInteger port = new AtomicInteger(0);

private ServiceRegistry<R> serviceRegistry;

protected AbstractDiscoveryLifecycle(ServiceRegistry<R> serviceRegistry) {
this.serviceRegistry = serviceRegistry;
}

protected ApplicationContext getContext() {
return context;
}
Expand Down Expand Up @@ -103,7 +111,7 @@ public void start() {
if (shouldRegisterManagement()) {
registerManagement();
}
this.context .publishEvent(new InstanceRegisteredEvent<>(this,
this.context.publishEvent(new InstanceRegisteredEvent<>(this,
getConfiguration()));
this.running.compareAndSet(false, true);
}
Expand All @@ -122,28 +130,44 @@ protected boolean shouldRegisterManagement() {
/**
* @return the object used to configure the DiscoveryClient

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or null when ____

*/
protected abstract Object getConfiguration();
protected Object getConfiguration() {
return null;
}

protected abstract R getRegistration();

protected abstract R getManagementRegistration();

protected ServiceRegistry<R> getServiceRegistry() {
return this.serviceRegistry;
}

/**
* Register the local service with the DiscoveryClient

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this doc need to be changed now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, DiscoveryClient was never right, since it wasn't used.

*/
protected abstract void register();
protected void register() {
this.serviceRegistry.register(getRegistration());
}

/**
* Register the local management service with the DiscoveryClient
*/
protected void registerManagement() {
this.serviceRegistry.register(getManagementRegistration());
}

/**
* De-register the local service with the DiscoveryClient

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

*/
protected abstract void deregister();
protected void deregister() {
this.serviceRegistry.deregister(getRegistration());
}

/**
* De-register the local management service with the DiscoveryClient

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

*/
protected void deregisterManagement() {
this.serviceRegistry.deregister(getManagementRegistration());
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package org.springframework.cloud.client.serviceregistry;

/**
* @author Spencer Gibb
*/
public interface Registration {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is a marker interface needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, cause looking at the test I don't really see any benefit of having this Registration interface. Can you explain that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, leftover from a refactor where this interface had a method and I removed it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up putting this back in, because I needed to lookup the registration in the spring context without knowing the implementation.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package org.springframework.cloud.client.serviceregistry;

/**
* @author Spencer Gibb
*/
public interface ServiceRegistry<R extends Registration> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here or somewhere else, we should qualify with an example of what a registration might be

void register(R registration);

void deregister(R registration);
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package org.springframework.cloud.client.discovery;

import static org.junit.Assert.*;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;

import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -9,6 +14,8 @@
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.test.SpringApplicationConfiguration;
import org.springframework.boot.test.WebIntegrationTest;
import org.springframework.cloud.client.serviceregistry.Registration;
import org.springframework.cloud.client.serviceregistry.ServiceRegistry;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
Expand Down Expand Up @@ -36,7 +43,9 @@ public void portsWork() {
assertNotEquals("Lifecycle port is management port", managementPort, lifecycle.getPort().get());
assertEquals("Lifecycle port is wrong", port, lifecycle.getPort().get());
assertTrue("Lifecycle not running", lifecycle.isRunning());
assertTrue("Lifecycle not registered", lifecycle.isRegistered());
assertThat("ServiceRegistry is wrong type", lifecycle.getServiceRegistry(), is(instanceOf(TestServiceRegistry.class)));
TestServiceRegistry serviceRegistry = (TestServiceRegistry) lifecycle.getServiceRegistry();
assertTrue("Lifecycle not registered", serviceRegistry.isRegistered());
assertEquals("Lifecycle appName is wrong", "application", lifecycle.getAppName());
}

Expand All @@ -49,11 +58,41 @@ public TestDiscoveryLifecycle testDiscoveryLifecycle() {
}
}

public static class TestDiscoveryLifecycle extends AbstractDiscoveryLifecycle {
public static class TestRegistration implements Registration {
}

public static class TestServiceRegistry implements ServiceRegistry<TestRegistration> {
private boolean registered = false;
private boolean deregistered = false;

@Override
public void register(TestRegistration registration) {
this.registered = true;
}

@Override
public void deregister(TestRegistration registration) {
this.deregistered = true;
}

public boolean isRegistered() {
return registered;
}

public boolean isDeregistered() {
return deregistered;
}
}

public static class TestDiscoveryLifecycle extends AbstractDiscoveryLifecycle<TestRegistration> {
private int port = 0;
private boolean registered = false;
private boolean deregistered = false;

protected TestDiscoveryLifecycle() {
super(new TestServiceRegistry());
}

@Override
protected int getConfiguredPort() {
return port;
Expand All @@ -65,31 +104,19 @@ protected void setConfiguredPort(int port) {
}

@Override
protected Object getConfiguration() {
return this;
}

@Override
protected void register() {
this.registered = true;
protected TestRegistration getRegistration() {
return null;
}

@Override
protected void deregister() {
this.deregistered = true;
protected TestRegistration getManagementRegistration() {
return null;
}

@Override
protected boolean isEnabled() {
return true;
}

public boolean isRegistered() {
return registered;
}

public boolean isDeregistered() {
return deregistered;
}
}
}