Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow specifying no default port when building PropertiesEndpointGroup #993

Merged
merged 1 commit into from
Feb 6, 2018
+151 −20
Merged

Allow specifying no default port when building PropertiesEndpointGroup #993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions core/src/main/java/com/linecorp/armeria/client/Endpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@

package com.linecorp.armeria.client;

import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;

import javax.annotation.Nullable;

import com.google.common.net.HostAndPort;

import com.linecorp.armeria.client.endpoint.EndpointGroupRegistry;
Expand Down Expand Up @@ -93,6 +96,7 @@ public static Endpoint of(String host, int port, int weight) {
private final String host;
private final int port;
private final int weight;
@Nullable
private String authority;

private Endpoint(String groupName) {
Expand Down Expand Up @@ -250,9 +254,7 @@ private void ensureSingle() {
}

private static void validatePort(String name, int port) {
if (port <= 0 || port > 65535) {
throw new IllegalArgumentException(name + ": " + port + " (expected: 1-65535)");
}
checkArgument(port > 0 && port <= 65535, "%s: %s (expected: 1-65535)", name, port);
}

private static void validateWeight(int weight) {
Expand Down
89 changes: 76 additions & 13 deletions core/src/main/java/com/linecorp/armeria/client/endpoint/PropertiesEndpointGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@
/**
* A {@link Properties} backed {@link EndpointGroup}. The list of {@link Endpoint}s are loaded from the
* {@link Properties}.
* TODO(ide) Reload the endpoint list if the file is updated.
*/
public final class PropertiesEndpointGroup implements EndpointGroup {

// TODO(ide) Reload the endpoint list if the file is updated.

/**
* Creates a new {@link EndpointGroup} instance that loads the host names (or IP address) and the port
* numbers of the {@link Endpoint} from the {@code resourceName} resource file. The resource file must
Expand All @@ -49,13 +51,40 @@ public final class PropertiesEndpointGroup implements EndpointGroup {
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param resourceName the resource file name that the list of {@link Endpoint}s loaded from
* @param resourceName the name of the resource where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
*
* @throws IllegalArgumentException if failed to load any hosts from the specified resource file
*/
public static PropertiesEndpointGroup of(ClassLoader classLoader, String resourceName,
String endpointKeyPrefix) {
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(classLoader, "classLoader"),
requireNonNull(resourceName, "resourceName"),
requireNonNull(endpointKeyPrefix, "endpointKeyPrefix"),
0));
}

/**
* Creates a new {@link EndpointGroup} instance that loads the host names (or IP address) and the port
* numbers of the {@link Endpoint} from the {@code resourceName} resource file. The resource file must
* contain at least one property whose name starts with {@code endpointKeyPrefix}:
*
* <pre>{@code
* example.hosts.0=example1.com:36462
* example.hosts.1=example2.com:36462
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param resourceName the name of the resource where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
* @param defaultPort the default port number to use
*
* @throws IllegalArgumentException if failed to load any hosts from the specified resource file
*/
public static PropertiesEndpointGroup of(ClassLoader classLoader, String resourceName,
String endpointKeyPrefix, int defaultPort) {
checkArgument(defaultPort >= 0 && defaultPort <= 65535,
"defaultPort(%s) must be between 0 and 65535", defaultPort);
validateDefaultPort(defaultPort);
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(classLoader, "classLoader"),
requireNonNull(resourceName, "resourceName"),
Expand All @@ -74,21 +103,49 @@ public static PropertiesEndpointGroup of(ClassLoader classLoader, String resourc
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param properties the {@link Properties} that the list of {@link Endpoint}s loaded from
* @param properties the {@link Properties} where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
*
* @throws IllegalArgumentException if failed to load any hosts from the specified {@link Properties}
*/
public static PropertiesEndpointGroup of(Properties properties, String endpointKeyPrefix) {
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(properties, "properties"),
requireNonNull(endpointKeyPrefix, "endpointKeyPrefix"),
0));
}

/**
* Creates a new {@link EndpointGroup} instance that loads the host names (or IP address) and the port
* numbers of the {@link Endpoint} from the {@link Properties}. The {@link Properties} must contain at
* least one property whose name starts with {@code endpointKeyPrefix}:
*
* <pre>{@code
* example.hosts.0=example1.com:36462
* example.hosts.1=example2.com:36462
* example.hosts.2=example3.com:36462
* }</pre>
*
* @param properties the {@link Properties} where the list of {@link Endpoint}s is loaded from
* @param endpointKeyPrefix the property name prefix
* @param defaultPort the default port number to use
*
* @throws IllegalArgumentException if failed to load any hosts from the specified {@link Properties}
*/
public static PropertiesEndpointGroup of(Properties properties, String endpointKeyPrefix,
int defaultPort) {
return new PropertiesEndpointGroup(loadEndpoints(properties, endpointKeyPrefix, defaultPort));
validateDefaultPort(defaultPort);
return new PropertiesEndpointGroup(loadEndpoints(
requireNonNull(properties, "properties"),
requireNonNull(endpointKeyPrefix, "endpointKeyPrefix"),
defaultPort));
}

private static List<Endpoint> loadEndpoints(ClassLoader classLoader, String resourceName,
String endpointKeyPrefix, int defaultPort) {
final URL resourceUrl = classLoader.getResource(resourceName);
if (resourceUrl == null) {
throw new IllegalArgumentException(resourceName + " not found");
}
if (endpointKeyPrefix.endsWith(".")) {
checkArgument(resourceUrl != null, "resource not found: %s", resourceName);
if (!endpointKeyPrefix.endsWith(".")) {
endpointKeyPrefix += ".";
}
try (InputStream in = resourceUrl.openStream()) {
Expand All @@ -110,14 +167,20 @@ private static List<Endpoint> loadEndpoints(Properties properties, String endpoi
if (key.startsWith(endpointKeyPrefix)) {
final Endpoint endpoint = Endpoint.parse(value);
checkState(!endpoint.isGroup(),
"%s contains an endpoint group which is not allowed: %s", properties, value);
newEndpoints.add(endpoint.withDefaultPort(defaultPort));
"properties contains an endpoint group which is not allowed: %s in %s",
value, properties);
newEndpoints.add(defaultPort == 0 ? endpoint : endpoint.withDefaultPort(defaultPort));
}
}
checkArgument(!newEndpoints.isEmpty(), "%s contains no hosts.", properties);
checkArgument(!newEndpoints.isEmpty(), "properties contains no hosts: %s", properties);
return ImmutableList.copyOf(newEndpoints);
}

private static void validateDefaultPort(int defaultPort) {
checkArgument(defaultPort > 0 && defaultPort <= 65535,
"defaultPort: %s (expected: 1-65535)", defaultPort);
}

private final List<Endpoint> endpoints;

private PropertiesEndpointGroup(List<Endpoint> endpoints) {
Expand Down
74 changes: 70 additions & 4 deletions core/src/test/java/com/linecorp/armeria/client/endpoint/PropertiesEndpointGroupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,92 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.util.Properties;

import org.junit.Test;

import com.linecorp.armeria.client.Endpoint;

public class PropertiesEndpointGroupTest {

private static final Properties PROPS = new Properties();

static {
PROPS.setProperty("serverA.hosts.0", "127.0.0.1:8080");
PROPS.setProperty("serverA.hosts.1", "127.0.0.1:8081");
PROPS.setProperty("serverA.hosts.2", "127.0.0.1");
PROPS.setProperty("serverB.hosts.0", "127.0.0.1:8082");
PROPS.setProperty("serverB.hosts.1", "127.0.0.1:8083");
}

@Test
public void propertiesWithoutDefaultPort() {
PropertiesEndpointGroup endpointGroup = PropertiesEndpointGroup.of(PROPS, "serverA.hosts");

assertThat(endpointGroup.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"),
Endpoint.of("127.0.0.1:8081"),
Endpoint.of("127.0.0.1"));
}

@Test
public void propertiesWithDefaultPort() {
PropertiesEndpointGroup endpointGroupA = PropertiesEndpointGroup.of(PROPS, "serverA.hosts", 80);
PropertiesEndpointGroup endpointGroupB = PropertiesEndpointGroup.of(PROPS, "serverB.hosts", 8080);

assertThat(endpointGroupA.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"),
Endpoint.of("127.0.0.1:8081"),
Endpoint.of("127.0.0.1:80"));
assertThat(endpointGroupB.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8082"),
Endpoint.of("127.0.0.1:8083"));
}

@Test
public void resourceWithoutDefaultPort() {
PropertiesEndpointGroup endpointGroup = PropertiesEndpointGroup.of(
getClass().getClassLoader(), "server-list.properties", "serverA.hosts");

assertThat(endpointGroup.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"),
Endpoint.of("127.0.0.1:8081"),
Endpoint.of("127.0.0.1"));
}

@Test
public void test() {
public void resourceWithDefaultPort() {
PropertiesEndpointGroup endpointGroupA = PropertiesEndpointGroup.of(
this.getClass().getClassLoader(), "server-list.properties", "serverA.hosts", 80);
getClass().getClassLoader(), "server-list.properties", "serverA.hosts", 80);
PropertiesEndpointGroup endpointGroupB = PropertiesEndpointGroup.of(
this.getClass().getClassLoader(), "server-list.properties", "serverB.hosts", 8080);
getClass().getClassLoader(), "server-list.properties", "serverB.hosts", 8080);

assertThat(endpointGroupA.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"),
Endpoint.of("127.0.0.1:8081"),
Endpoint.of("127.0.0.1:80"));
assertThat(endpointGroupB.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8082"),
Endpoint.of("127.0.0.1:8083"));
}

@Test
public void testWithPrefixThatEndsWithDot() {
PropertiesEndpointGroup endpointGroup = PropertiesEndpointGroup.of(
getClass().getClassLoader(), "server-list.properties", "serverA.hosts.");

assertThat(endpointGroup.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"),
Endpoint.of("127.0.0.1:8081"),
Endpoint.of("127.0.0.1"));
}

@Test
public void containsNoHosts() {
assertThatThrownBy(() -> PropertiesEndpointGroup.of(
this.getClass().getClassLoader(), "server-list.properties", "serverC.hosts", 8080))
getClass().getClassLoader(), "server-list.properties", "serverC.hosts", 8080))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("contains no hosts");
}

@Test
public void illegalDefaultPort() {
assertThatThrownBy(() -> PropertiesEndpointGroup.of(
getClass().getClassLoader(), "server-list.properties", "serverA.hosts", 0))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("defaultPort");
}
}