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.

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

Excavator: Format Java files #1285

Closed
wants to merge 12 commits into from
4 changes: 0 additions & 4 deletions .baseline/checkstyle/checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,6 @@
<property name="separated" value="true"/>
<property name="sortStaticImportsAlphabetically" value="true"/>
</module>
<module name="Indentation"> <!-- Java Style Guide: Block indentation: +4 spaces -->
<property name="arrayInitIndent" value="8"/>
<property name="lineWrappingIndentation" value="8"/>
</module>
<module name="InnerAssignment"/> <!-- Java Coding Guidelines: Inner assignments: Not used -->
<module name="LeftCurly"/> <!-- Java Style Guide: Nonempty blocks: K & R style -->
<module name="LineLength"> <!-- Java Style Guide: No line-wrapping -->
Expand Down
319 changes: 0 additions & 319 deletions .baseline/spotless/eclipse.xml

This file was deleted.

1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ buildscript {
classpath 'com.palantir.gradle.gitversion:gradle-git-version:0.12.2'
classpath 'gradle.plugin.org.inferred:gradle-processors:3.1.0'
classpath 'com.palantir.gradle.consistentversions:gradle-consistent-versions:1.12.4'
classpath 'com.palantir.javaformat:gradle-palantir-java-format:0.3.2'
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ public final class CipherSuites {

/**
* Known fast and safe cipher suites on the JVM.
* <p>
* In an ideal world, we'd use GCM suites, but they're an order of
* magnitude slower than the CBC suites, which have JVM optimizations
* already. We should revisit with JDK9.
* <p>
* See also:
*
* <p>In an ideal world, we'd use GCM suites, but they're an order of magnitude slower than the CBC suites, which
* have JVM optimizations already. We should revisit with JDK9.
*
* <p>See also:
*
* <ul>
* <li>http://openjdk.java.net/jeps/246
* <li>https://bugs.openjdk.java.net/secure/attachment/25422/GCM%20Analysis.pdf
* <li>http://openjdk.java.net/jeps/246
* <li>https://bugs.openjdk.java.net/secure/attachment/25422/GCM%20Analysis.pdf
* </ul>
*/
public static String[] fastCipherSuites() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,21 @@ public interface ClientConfiguration {
* configured {@link #uris uri}; requests carry an additional {@code Host} header (or http/2 {@code :authority}
* pseudo-header) set to the configured {@link #uris uri}. The proxy is expected to forward such requests to the
* original {@code #uris uri}.
* <p>
* Note that if this option is set, then the {@link #maxNumRetries} must also be set to 0 and exactly one {@link
*
* <p>Note that if this option is set, then the {@link #maxNumRetries} must also be set to 0 and exactly one {@link
* #uris} must exist since the mesh proxy is expected to handle all retry logic.
*/
Optional<HostAndPort> meshProxy();

/** The maximum number of times a failed request is retried. */
int maxNumRetries();

/**
* Indicates how the target node is selected for a given request.
*/
/** Indicates how the target node is selected for a given request. */
NodeSelectionStrategy nodeSelectionStrategy();

/**
* The amount of time a URL marked as failed should be avoided for subsequent calls. If the
* {@link #nodeSelectionStrategy} is ROUND_ROBIN, this must be a positive period of time.
* The amount of time a URL marked as failed should be avoided for subsequent calls. If the {@link
* #nodeSelectionStrategy} is ROUND_ROBIN, this must be a positive period of time.
*/
Duration failedUrlCooldown();

Expand Down Expand Up @@ -126,7 +124,8 @@ default void check() {
checkArgument(uris().size() == 1, "If meshProxy is configured then uris must contain exactly 1 URI");
}
if (nodeSelectionStrategy().equals(NodeSelectionStrategy.ROUND_ROBIN)) {
checkArgument(!failedUrlCooldown().isNegative() && !failedUrlCooldown().isZero(),
checkArgument(
!failedUrlCooldown().isNegative() && !failedUrlCooldown().isZero(),
"If nodeSelectionStrategy is ROUND_ROBIN then failedUrlCooldown must be positive");
}
// Assert that timeouts are in milliseconds, not any higher precision, because feign only supports millis.
Expand All @@ -136,7 +135,8 @@ default void check() {
}

default void checkTimeoutPrecision(Duration duration, String timeoutName) {
checkArgument(duration.minusMillis(duration.toMillis()).isZero(),
checkArgument(
duration.minusMillis(duration.toMillis()).isZero(),
"Timeouts with sub-millisecond precision are not supported",
SafeArg.of("timeoutName", timeoutName),
SafeArg.of("duration", duration),
Expand All @@ -154,9 +154,9 @@ enum ClientQoS {
ENABLED,

/**
* Disables the client-side sympathetic QoS. Consumers should almost never use this option, reserving it
* for where there are known issues with the QoS interaction. Please consult project maintainers if applying
* this option.
* Disables the client-side sympathetic QoS. Consumers should almost never use this option, reserving it for
* where there are known issues with the QoS interaction. Please consult project maintainers if applying this
* option.
*/
DANGEROUS_DISABLE_SYMPATHETIC_CLIENT_QOS
}
Expand All @@ -166,20 +166,20 @@ enum ServerQoS {
AUTOMATIC_RETRY,

/**
* Propagate QosException.Throttle and QosException.Unavailable (429/503) to the caller. Consumers
* should use this when an upstream service has better context on how to handle the QoS error. This delegates
* the responsibility to the upstream service, which should use an appropriate conjure client to handle the
* Propagate QosException.Throttle and QosException.Unavailable (429/503) to the caller. Consumers should use
* this when an upstream service has better context on how to handle the QoS error. This delegates the
* responsibility to the upstream service, which should use an appropriate conjure client to handle the
* response.
*
* For example, let us imagine a proxy server that serves both interactive and long-running background requests
* by dispatching requests to some backend. Interactive requests should be retried relatively few times in
* comparison to background jobs which run for minutes our even hours. The proxy server should use a backend
* <p>For example, let us imagine a proxy server that serves both interactive and long-running background
* requests by dispatching requests to some backend. Interactive requests should be retried relatively few times
* in comparison to background jobs which run for minutes our even hours. The proxy server should use a backend
* client that propagates the QoS responses instead of retrying so the proxy client can handle them
* appropriately. There is no risk of retry storms because the retries are isolated to one layer, the proxy
* client.
*
* Note that QosException.RetryOther (308) is not propagated. If the proxy server is exposed on the front door
* but the backend is not, it makes no sense to redirect the caller to a new backend. The client will still
* <p>Note that QosException.RetryOther (308) is not propagated. If the proxy server is exposed on the front
* door but the backend is not, it makes no sense to redirect the caller to a new backend. The client will still
* follow redirects.
*/
PROPAGATE_429_and_503_TO_CALLER
Expand All @@ -193,11 +193,11 @@ enum RetryOnTimeout {
* Consumers should almost never use this option, reserving it for when their clients have guaranteed that they
* will never retry on timeout.
*
* The risk of retry storms is severe. If the client times out before the consumer finishes retrying on its
* <p>The risk of retry storms is severe. If the client times out before the consumer finishes retrying on its
* timeout, it will submit a new request which will also retry on timeout. Expensive requests that time out have
* brought down servers with this behavior enabled.
*
* Note that connect timeouts will always be retried.
* <p>Note that connect timeouts will always be retried.
*/
DANGEROUS_ENABLE_AT_RISK_OF_RETRY_STORMS
}
Expand All @@ -206,12 +206,11 @@ enum RetryOnSocketException {
/** Default. */
ENABLED,
/**
* Disables all {@link java.net.SocketException} handling. This is almost always not what you want,
* the solitary case where this is desirable being cases where Conjure is used to create single-host clients
* with retry on host failure handled outside of the Conjure layer. If you want to use this, please
* talk to a relevant party; this is here to enable a very specific workflow.
* Disables all {@link java.net.SocketException} handling. This is almost always not what you want, the solitary
* case where this is desirable being cases where Conjure is used to create single-host clients with retry on
* host failure handled outside of the Conjure layer. If you want to use this, please talk to a relevant party;
* this is here to enable a very specific workflow.
*/
DANGEROUS_DISABLED
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ public final class ClientConfigurations {
private static final boolean DEFAULT_FALLBACK_TO_COMMON_NAME_VERIFICATION = false;
private static final NodeSelectionStrategy DEFAULT_NODE_SELECTION_STRATEGY = NodeSelectionStrategy.PIN_UNTIL_ERROR;
private static final int DEFAULT_MAX_NUM_RETRIES = 4;
private static final ClientConfiguration.ClientQoS CLIENT_QOS_DEFAULT =
ClientConfiguration.ClientQoS.ENABLED;
private static final ClientConfiguration.ClientQoS CLIENT_QOS_DEFAULT = ClientConfiguration.ClientQoS.ENABLED;
private static final ClientConfiguration.ServerQoS PROPAGATE_QOS_DEFAULT =
ClientConfiguration.ServerQoS.AUTOMATIC_RETRY;
private static final ClientConfiguration.RetryOnTimeout RETRY_ON_TIMEOUT_DEFAULT =
Expand All @@ -73,11 +72,10 @@ public static ClientConfiguration of(ServiceConfiguration config) {
.readTimeout(config.readTimeout().orElse(DEFAULT_READ_TIMEOUT))
.writeTimeout(config.writeTimeout().orElse(DEFAULT_WRITE_TIMEOUT))
.enableGcmCipherSuites(config.enableGcmCipherSuites().orElse(DEFAULT_ENABLE_GCM_CIPHERS))
.fallbackToCommonNameVerification(config.fallbackToCommonNameVerification()
.orElse(DEFAULT_FALLBACK_TO_COMMON_NAME_VERIFICATION))
.proxy(config.proxy()
.map(ClientConfigurations::createProxySelector)
.orElseGet(ProxySelector::getDefault))
.fallbackToCommonNameVerification(
config.fallbackToCommonNameVerification().orElse(DEFAULT_FALLBACK_TO_COMMON_NAME_VERIFICATION))
.proxy(config.proxy().map(ClientConfigurations::createProxySelector).orElseGet(ProxySelector
::getDefault))
Copy link
Contributor

Choose a reason for hiding this comment

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

so.. this is the kind of sadness enabled by the quick hack in palantir/palantir-java-format#44

.proxyCredentials(config.proxy().flatMap(ProxyConfiguration::credentials))
.meshProxy(meshProxy(config.proxy()))
.maxNumRetries(config.maxNumRetries().orElse(DEFAULT_MAX_NUM_RETRIES))
Expand All @@ -97,9 +95,7 @@ public static ClientConfiguration of(ServiceConfiguration config) {
* other configuration with the defaults specified as constants in this class.
*/
public static ClientConfiguration of(
List<String> uris,
SSLSocketFactory sslSocketFactory,
X509TrustManager trustManager) {
List<String> uris, SSLSocketFactory sslSocketFactory, X509TrustManager trustManager) {
return ClientConfiguration.builder()
.sslSocketFactory(new KeepAliveSslSocketFactory(sslSocketFactory))
.trustManager(trustManager)
Expand Down Expand Up @@ -128,8 +124,8 @@ public static ProxySelector createProxySelector(ProxyConfiguration proxyConfig)
case DIRECT:
return fixedProxySelectorFor(Proxy.NO_PROXY);
case HTTP:
HostAndPort hostAndPort = HostAndPort.fromString(proxyConfig.hostAndPort()
.orElseThrow(() -> new SafeIllegalArgumentException(
HostAndPort hostAndPort = HostAndPort.fromString(
proxyConfig.hostAndPort().orElseThrow(() -> new SafeIllegalArgumentException(
"Expected to find proxy hostAndPort configuration for HTTP proxy")));
InetSocketAddress addr = new InetSocketAddress(hostAndPort.getHost(), hostAndPort.getPort());
return fixedProxySelectorFor(new Proxy(Proxy.Type.HTTP, addr));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,8 @@ public Socket createSocket(InetAddress host, int port) throws IOException {
}

@Override
public Socket createSocket(
InetAddress address,
int port,
InetAddress localAddress,
int localPort) throws IOException {
public Socket createSocket(InetAddress address, int port, InetAddress localAddress, int localPort)
throws IOException {
return wrap(getDelegate().createSocket(address, port, localAddress, localPort));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,5 @@

@Target({ElementType.PACKAGE, ElementType.TYPE})
@Retention(RetentionPolicy.CLASS)
@Value.Style(
visibility = Value.Style.ImplementationVisibility.PACKAGE,
overshadowImplementation = true,
jdkOnly = true)
@Value.Style(visibility = Value.Style.ImplementationVisibility.PACKAGE, overshadowImplementation = true, jdkOnly = true)
public @interface ImmutablesStyle {}
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,10 @@
import javax.net.ssl.SSLSocketFactory;

/**
* Enables {@link java.net.StandardSocketOptions#SO_KEEPALIVE}. By default this will only send a packet after two
* hours (which is long after our timeouts), but this can be lowered to be useful in the kernel, e.g.:
* Enables {@link java.net.StandardSocketOptions#SO_KEEPALIVE}. By default this will only send a packet after two hours
* (which is long after our timeouts), but this can be lowered to be useful in the kernel, e.g.:
*
* net.ipv4.tcp_keepalive_time = 20
* net.ipv4.tcp_keepalive_intvl = 5
* net.ipv4.tcp_keepalive_probes = 3
* <p>net.ipv4.tcp_keepalive_time = 20 net.ipv4.tcp_keepalive_intvl = 5 net.ipv4.tcp_keepalive_probes = 3
*/
final class KeepAliveSslSocketFactory extends ForwardingSslSocketFactory {
private final SSLSocketFactory delegate;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,9 @@ public enum NodeSelectionStrategy {
*/
PIN_UNTIL_ERROR,

/**
* For each new request, select the "next" node (in some undefined order).
*/
/** For each new request, select the "next" node (in some undefined order). */
ROUND_ROBIN,

/**
* Similar to {@link #PIN_UNTIL_ERROR}, except will not shuffle the URLs throughout the lifetime of the client.
*/
/** Similar to {@link #PIN_UNTIL_ERROR}, except will not shuffle the URLs throughout the lifetime of the client. */
PIN_UNTIL_ERROR_WITHOUT_RESHUFFLE
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ public void testFromParameters_fillsInDefaults() {
ClientConfiguration actual = ClientConfigurations.of(uris, sslFactory, trustManager);

assertThat(actual.sslSocketFactory()).isInstanceOf(KeepAliveSslSocketFactory.class);
assertThat(((KeepAliveSslSocketFactory) actual.sslSocketFactory()).getDelegate()).isEqualTo(sslFactory);
assertThat(((KeepAliveSslSocketFactory) actual.sslSocketFactory()).getDelegate())
.isEqualTo(sslFactory);
Copy link
Contributor

@dansanduleac dansanduleac Nov 12, 2019

Choose a reason for hiding this comment

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

@iamdanfox @ferozco 0.3.1 made a lot of the fluent assertions line break.
I don't think that's necessarily bad though, it makes it a lot more obvious that there is a method call without having to look all the way to the right for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the additional break is fine

assertThat(actual.trustManager()).isEqualTo(trustManager);
assertThat(actual.uris()).isEqualTo(uris);
assertThat(actual.connectTimeout()).isEqualTo(Duration.ofSeconds(10));
Expand All @@ -89,8 +90,7 @@ public void testTimeoutMustBeMilliseconds() {
.security(SslConfiguration.of(Paths.get("src/test/resources/trustStore.jks")))
.connectTimeout(Duration.ofNanos(5))
.build();
Assertions
.assertThatLoggableExceptionThrownBy(() -> ClientConfigurations.of(serviceConfig))
Assertions.assertThatLoggableExceptionThrownBy(() -> ClientConfigurations.of(serviceConfig))
.hasLogMessage("Timeouts with sub-millisecond precision are not supported");
}

Expand Down Expand Up @@ -121,13 +121,12 @@ public void roundRobin_noCooldown() throws Exception {
.build();

assertThatThrownBy(() -> ClientConfiguration.builder()
.from(
ClientConfigurations.of(serviceConfig))
.nodeSelectionStrategy(NodeSelectionStrategy.ROUND_ROBIN)
.failedUrlCooldown(Duration.ofMillis(0))
.build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("If nodeSelectionStrategy is ROUND_ROBIN then failedUrlCooldown must be positive");
.from(ClientConfigurations.of(serviceConfig))
.nodeSelectionStrategy(NodeSelectionStrategy.ROUND_ROBIN)
.failedUrlCooldown(Duration.ofMillis(0))
.build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("If nodeSelectionStrategy is ROUND_ROBIN then failedUrlCooldown must be positive");
Copy link
Contributor

Choose a reason for hiding this comment

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

this is much nicer, exactly how I would have formatted it myself!

}

@Test
Expand Down Expand Up @@ -178,5 +177,4 @@ private ServiceConfiguration meshProxyServiceConfig(List<String> theUris, int ma
.maxNumRetries(maxNumRetries)
.build();
}

}
Loading