From e9779d7c00cde14b33ba3239c859f997e70c2b2e Mon Sep 17 00:00:00 2001 From: Carl Mastrangelo Date: Fri, 2 Dec 2016 15:35:11 -0800 Subject: [PATCH] all: use a proper log id which can reference channels, subchannels, and transports --- .../io/grpc/inprocess/InProcessTransport.java | 7 +- .../grpc/internal/DelayedClientTransport.java | 8 ++- .../ForwardingConnectionClientTransport.java | 2 +- .../main/java/io/grpc/internal/GrpcUtil.java | 7 -- .../io/grpc/internal/InternalSubchannel.java | 20 +++--- .../src/main/java/io/grpc/internal/LogId.java | 69 +++++++++++++++++++ .../io/grpc/internal/ManagedChannelImpl.java | 5 +- .../java/io/grpc/internal/TransportSet.java | 5 +- .../main/java/io/grpc/internal/WithLogId.java | 2 +- .../grpc/internal/InternalSubchannelTest.java | 10 +-- .../io/grpc/internal/TransportSetTest.java | 4 +- .../io/grpc/netty/NettyClientTransport.java | 7 +- .../io/grpc/okhttp/OkHttpClientTransport.java | 6 +- 13 files changed, 114 insertions(+), 38 deletions(-) create mode 100644 core/src/main/java/io/grpc/internal/LogId.java diff --git a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java index 31f5bbc3e0..051586632f 100644 --- a/core/src/main/java/io/grpc/inprocess/InProcessTransport.java +++ b/core/src/main/java/io/grpc/inprocess/InProcessTransport.java @@ -44,7 +44,7 @@ import io.grpc.Status; import io.grpc.internal.ClientStream; import io.grpc.internal.ClientStreamListener; import io.grpc.internal.ConnectionClientTransport; -import io.grpc.internal.GrpcUtil; +import io.grpc.internal.LogId; import io.grpc.internal.ManagedClientTransport; import io.grpc.internal.NoopClientStream; import io.grpc.internal.ServerStream; @@ -71,6 +71,7 @@ import javax.annotation.concurrent.ThreadSafe; class InProcessTransport implements ServerTransport, ConnectionClientTransport { private static final Logger log = Logger.getLogger(InProcessTransport.class.getName()); + private final LogId logId = LogId.allocate(getClass().getName()); private final String name; private ServerTransportListener serverTransportListener; private Attributes serverStreamAttributes; @@ -203,8 +204,8 @@ class InProcessTransport implements ServerTransport, ConnectionClientTransport { } @Override - public String getLogId() { - return GrpcUtil.getLogId(this); + public LogId getLogId() { + return logId; } @Override diff --git a/core/src/main/java/io/grpc/internal/DelayedClientTransport.java b/core/src/main/java/io/grpc/internal/DelayedClientTransport.java index d4df6f8294..02eb92af42 100644 --- a/core/src/main/java/io/grpc/internal/DelayedClientTransport.java +++ b/core/src/main/java/io/grpc/internal/DelayedClientTransport.java @@ -63,6 +63,9 @@ import javax.annotation.concurrent.GuardedBy; * streams are transferred to the given transport, thus this transport won't own any stream. */ class DelayedClientTransport implements ManagedClientTransport { + + private final LogId lodId = LogId.allocate(getClass().getName()); + private final Object lock = new Object(); private final Executor streamCreationExecutor; @@ -455,9 +458,10 @@ class DelayedClientTransport implements ManagedClientTransport { } } + // TODO(carl-mastrangelo): remove this once the Subchannel change is in. @Override - public final String getLogId() { - return GrpcUtil.getLogId(this); + public LogId getLogId() { + return lodId; } @VisibleForTesting diff --git a/core/src/main/java/io/grpc/internal/ForwardingConnectionClientTransport.java b/core/src/main/java/io/grpc/internal/ForwardingConnectionClientTransport.java index b5e1e25e18..6dfe53c9bb 100644 --- a/core/src/main/java/io/grpc/internal/ForwardingConnectionClientTransport.java +++ b/core/src/main/java/io/grpc/internal/ForwardingConnectionClientTransport.java @@ -73,7 +73,7 @@ abstract class ForwardingConnectionClientTransport implements ConnectionClientTr } @Override - public String getLogId() { + public LogId getLogId() { return delegate().getLogId(); } diff --git a/core/src/main/java/io/grpc/internal/GrpcUtil.java b/core/src/main/java/io/grpc/internal/GrpcUtil.java index cebf1de5cd..cc7ebbabae 100644 --- a/core/src/main/java/io/grpc/internal/GrpcUtil.java +++ b/core/src/main/java/io/grpc/internal/GrpcUtil.java @@ -535,13 +535,6 @@ public final class GrpcUtil { } } - /** - * The canonical implementation of {@link WithLogId#getLogId}. - */ - public static String getLogId(WithLogId subject) { - return subject.getClass().getSimpleName() + "@" + Integer.toHexString(subject.hashCode()); - } - private GrpcUtil() {} private static String getImplementationVersion() { diff --git a/core/src/main/java/io/grpc/internal/InternalSubchannel.java b/core/src/main/java/io/grpc/internal/InternalSubchannel.java index 54fd486a8d..edb9fc007e 100644 --- a/core/src/main/java/io/grpc/internal/InternalSubchannel.java +++ b/core/src/main/java/io/grpc/internal/InternalSubchannel.java @@ -72,7 +72,9 @@ import javax.annotation.concurrent.ThreadSafe; final class InternalSubchannel implements WithLogId { private static final Logger log = Logger.getLogger(InternalSubchannel.class.getName()); + private final Object lock = new Object(); + private final LogId logId = LogId.allocate(getClass().getName()); private final EquivalentAddressGroup addressGroup; private final String authority; private final String userAgent; @@ -119,12 +121,12 @@ final class InternalSubchannel implements WithLogId { * also be present. */ @GuardedBy("lock") - private final Collection transports = - new ArrayList(); + private final Collection transports = + new ArrayList(); // Must only be used from channelExecutor - private final InUseStateAggregator2 inUseStateAggregator = - new InUseStateAggregator2() { + private final InUseStateAggregator2 inUseStateAggregator = + new InUseStateAggregator2() { @Override void handleInUse() { callback.onInUse(InternalSubchannel.this); @@ -329,7 +331,7 @@ final class InternalSubchannel implements WithLogId { } private void handleTransportInUseState( - final ManagedClientTransport transport, final boolean inUse) { + final ConnectionClientTransport transport, final boolean inUse) { channelExecutor.execute(new Runnable() { @Override public void run() { @@ -358,8 +360,8 @@ final class InternalSubchannel implements WithLogId { } @Override - public String getLogId() { - return GrpcUtil.getLogId(this); + public LogId getLogId() { + return logId; } @VisibleForTesting @@ -371,10 +373,10 @@ final class InternalSubchannel implements WithLogId { /** Listener for real transports. */ private class TransportListener implements ManagedClientTransport.Listener { - final ManagedClientTransport transport; + final ConnectionClientTransport transport; final SocketAddress address; - TransportListener(ManagedClientTransport transport, SocketAddress address) { + TransportListener(ConnectionClientTransport transport, SocketAddress address) { this.transport = transport; this.address = address; } diff --git a/core/src/main/java/io/grpc/internal/LogId.java b/core/src/main/java/io/grpc/internal/LogId.java new file mode 100644 index 0000000000..c9cff26c0d --- /dev/null +++ b/core/src/main/java/io/grpc/internal/LogId.java @@ -0,0 +1,69 @@ +/* + * Copyright 2016, Google Inc. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package io.grpc.internal; + +import java.util.concurrent.atomic.AtomicLong; + +/** + * A loggable ID, unique for the duration of the program. + */ +public final class LogId { + private static final AtomicLong idAlloc = new AtomicLong(); + + /** + * @param tag a loggable tag associated with this ID. + */ + public static LogId allocate(String tag) { + return new LogId(tag, idAlloc.incrementAndGet()); + } + + private final String tag; + private final long id; + + private LogId(String tag, long id) { + this.tag = tag; + this.id = id; + } + + public long getId() { + return id; + } + + public String getTag() { + return tag; + } + + @Override + public String toString() { + return tag + "-" + id; + } +} diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index bba906689e..ee416482c5 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -130,6 +130,7 @@ public final class ManagedChannelImpl extends ManagedChannel implements WithLogI private final Executor executor; private final boolean usingSharedExecutor; private final Object lock = new Object(); + private final LogId logId = LogId.allocate(getClass().getName()); private final DecompressorRegistry decompressorRegistry; private final CompressorRegistry compressorRegistry; @@ -745,8 +746,8 @@ public final class ManagedChannelImpl extends ManagedChannel implements WithLogI }; @Override - public String getLogId() { - return GrpcUtil.getLogId(this); + public LogId getLogId() { + return logId; } private static class NameResolverListenerImpl implements NameResolver.Listener { diff --git a/core/src/main/java/io/grpc/internal/TransportSet.java b/core/src/main/java/io/grpc/internal/TransportSet.java index dfbdf82b11..9ab5b63663 100644 --- a/core/src/main/java/io/grpc/internal/TransportSet.java +++ b/core/src/main/java/io/grpc/internal/TransportSet.java @@ -73,6 +73,7 @@ final class TransportSet extends ManagedChannel implements WithLogId { private final CountDownLatch terminatedLatch = new CountDownLatch(1); private final Object lock = new Object(); + private final LogId logId = LogId.allocate(getClass().getName()); private final EquivalentAddressGroup addressGroup; private final String authority; private final String userAgent; @@ -352,8 +353,8 @@ final class TransportSet extends ManagedChannel implements WithLogId { } @Override - public String getLogId() { - return GrpcUtil.getLogId(this); + public LogId getLogId() { + return logId; } @Override diff --git a/core/src/main/java/io/grpc/internal/WithLogId.java b/core/src/main/java/io/grpc/internal/WithLogId.java index bbf3d605bb..98d579d132 100644 --- a/core/src/main/java/io/grpc/internal/WithLogId.java +++ b/core/src/main/java/io/grpc/internal/WithLogId.java @@ -43,5 +43,5 @@ public interface WithLogId { *

The subclasses of this interface usually want to include the log ID in their {@link * #toString} results. */ - String getLogId(); + LogId getLogId(); } diff --git a/core/src/test/java/io/grpc/internal/InternalSubchannelTest.java b/core/src/test/java/io/grpc/internal/InternalSubchannelTest.java index 9b5eccdf4f..2359ba2969 100644 --- a/core/src/test/java/io/grpc/internal/InternalSubchannelTest.java +++ b/core/src/test/java/io/grpc/internal/InternalSubchannelTest.java @@ -422,7 +422,7 @@ public class InternalSubchannelTest { // No scheduled tasks that would ever try to reconnect ... assertEquals(0, fakeClock.numPendingTasks()); assertEquals(0, fakeExecutor.numPendingTasks()); - + // ... until it's requested. internalSubchannel.obtainActiveTransport(); assertExactCallbackInvokes("onStateChange:CONNECTING"); @@ -434,7 +434,7 @@ public class InternalSubchannelTest { public void shutdownWhenReady() throws Exception { SocketAddress addr = mock(SocketAddress.class); createInternalSubchannel(addr); - + internalSubchannel.obtainActiveTransport(); MockClientTransportInfo transportInfo = transports.poll(); transportInfo.listener.transportReady(); @@ -528,7 +528,7 @@ public class InternalSubchannelTest { public void shutdownNow() throws Exception { SocketAddress addr = mock(SocketAddress.class); createInternalSubchannel(addr); - + internalSubchannel.obtainActiveTransport(); MockClientTransportInfo t1 = transports.poll(); t1.listener.transportReady(); @@ -565,8 +565,8 @@ public class InternalSubchannelTest { @Test public void logId() { createInternalSubchannel(mock(SocketAddress.class)); - assertEquals("InternalSubchannel@" + Integer.toHexString(internalSubchannel.hashCode()), - internalSubchannel.getLogId()); + + assertNotNull(internalSubchannel.getLogId()); } @Test diff --git a/core/src/test/java/io/grpc/internal/TransportSetTest.java b/core/src/test/java/io/grpc/internal/TransportSetTest.java index e3088a6d64..28374e65dd 100644 --- a/core/src/test/java/io/grpc/internal/TransportSetTest.java +++ b/core/src/test/java/io/grpc/internal/TransportSetTest.java @@ -690,8 +690,8 @@ public class TransportSetTest { @Test public void logId() { createTransportSet(mock(SocketAddress.class)); - assertEquals("TransportSet@" + Integer.toHexString(transportSet.hashCode()), - transportSet.getLogId()); + + assertNotNull(transportSet.getLogId()); } @Test diff --git a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java index 32bff4639b..9297c549b5 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java @@ -46,6 +46,7 @@ import io.grpc.internal.ClientStream; import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.GrpcUtil; import io.grpc.internal.Http2Ping; +import io.grpc.internal.LogId; import io.grpc.internal.StatsTraceContext; import io.netty.bootstrap.Bootstrap; import io.netty.channel.Channel; @@ -62,12 +63,14 @@ import java.net.SocketAddress; import java.nio.channels.ClosedChannelException; import java.util.Map; import java.util.concurrent.Executor; + import javax.annotation.Nullable; /** * A Netty-based {@link ConnectionClientTransport} implementation. */ class NettyClientTransport implements ConnectionClientTransport { + private final LogId logId = LogId.allocate(getClass().getName()); private final Map, ?> channelOptions; private final SocketAddress address; private final Class channelType; @@ -241,8 +244,8 @@ class NettyClientTransport implements ConnectionClientTransport { } @Override - public String getLogId() { - return GrpcUtil.getLogId(this); + public LogId getLogId() { + return logId; } @Override diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java index 7bc91d9567..2649b63c9e 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java @@ -51,6 +51,7 @@ import io.grpc.internal.ConnectionClientTransport; import io.grpc.internal.GrpcUtil; import io.grpc.internal.Http2Ping; import io.grpc.internal.KeepAliveManager; +import io.grpc.internal.LogId; import io.grpc.internal.SerializingExecutor; import io.grpc.internal.SharedResourceHolder; import io.grpc.internal.StatsTraceContext; @@ -140,6 +141,7 @@ class OkHttpClientTransport implements ConnectionClientTransport { private AsyncFrameWriter frameWriter; private OutboundFlowController outboundFlow; private final Object lock = new Object(); + private final LogId logId = LogId.allocate(getClass().getName()); @GuardedBy("lock") private int nextStreamId; @GuardedBy("lock") @@ -445,8 +447,8 @@ class OkHttpClientTransport implements ConnectionClientTransport { } @Override - public String getLogId() { - return GrpcUtil.getLogId(this); + public LogId getLogId() { + return logId; } /**