You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by jd...@apache.org on 2017/04/04 19:45:22 UTC
[1/2] kudu git commit: KUDU-1939. Remove WARNING about large arenas
Repository: kudu
Updated Branches:
refs/heads/master 2407c2cb1 -> ac6e24fc2
KUDU-1939. Remove WARNING about large arenas
This WARNING turned out to be scary to users and hasn't been
particularly useful for us in recent years. This patch just removes it.
Change-Id: I8e98cc9ff4947ecd5970b5cc56d97d248ca1de7e
Reviewed-on: http://gerrit.cloudera.org:8080/6542
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/2bc2c322
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/2bc2c322
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/2bc2c322
Branch: refs/heads/master
Commit: 2bc2c322e7be7af456ce26b0022b35dff8e654d6
Parents: 2407c2c
Author: Todd Lipcon <to...@apache.org>
Authored: Tue Apr 4 11:42:38 2017 -0700
Committer: Jean-Daniel Cryans <jd...@apache.org>
Committed: Tue Apr 4 19:42:51 2017 +0000
----------------------------------------------------------------------
src/kudu/util/memory/arena.cc | 18 ++----------------
src/kudu/util/memory/arena.h | 4 ----
2 files changed, 2 insertions(+), 20 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/2bc2c322/src/kudu/util/memory/arena.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/memory/arena.cc b/src/kudu/util/memory/arena.cc
index 106bd75..427072f 100644
--- a/src/kudu/util/memory/arena.cc
+++ b/src/kudu/util/memory/arena.cc
@@ -33,10 +33,6 @@ using std::sort;
using std::swap;
using std::unique_ptr;
-DEFINE_int64(arena_warn_threshold_bytes, 256*1024*1024,
- "Number of bytes beyond which to emit a warning for a large arena");
-TAG_FLAG(arena_warn_threshold_bytes, hidden);
-
namespace kudu {
template <bool THREADSAFE>
@@ -49,8 +45,7 @@ ArenaBase<THREADSAFE>::ArenaBase(
size_t max_buffer_size)
: buffer_allocator_(buffer_allocator),
max_buffer_size_(max_buffer_size),
- arena_footprint_(0),
- warned_(false) {
+ arena_footprint_(0) {
AddComponent(CHECK_NOTNULL(NewComponent(initial_buffer_size, 0)));
}
@@ -58,8 +53,7 @@ template <bool THREADSAFE>
ArenaBase<THREADSAFE>::ArenaBase(size_t initial_buffer_size, size_t max_buffer_size)
: buffer_allocator_(HeapBufferAllocator::Get()),
max_buffer_size_(max_buffer_size),
- arena_footprint_(0),
- warned_(false) {
+ arena_footprint_(0) {
AddComponent(CHECK_NOTNULL(NewComponent(initial_buffer_size, 0)));
}
@@ -128,13 +122,6 @@ void ArenaBase<THREADSAFE>::AddComponent(ArenaBase::Component *component) {
ReleaseStoreCurrent(component);
arena_.push_back(unique_ptr<Component>(component));
arena_footprint_ += component->size();
- if (PREDICT_FALSE(arena_footprint_ > FLAGS_arena_warn_threshold_bytes) && !warned_) {
- LOG(WARNING) << "Arena " << reinterpret_cast<const void *>(this)
- << " footprint (" << arena_footprint_ << " bytes) exceeded warning threshold ("
- << FLAGS_arena_warn_threshold_bytes << " bytes)\n"
- << GetStackTrace();
- warned_ = true;
- }
}
template <bool THREADSAFE>
@@ -149,7 +136,6 @@ void ArenaBase<THREADSAFE>::Reset() {
}
arena_.back()->Reset();
arena_footprint_ = arena_.back()->size();
- warned_ = false;
#ifndef NDEBUG
// In debug mode release the last component too for (hopefully) better
http://git-wip-us.apache.org/repos/asf/kudu/blob/2bc2c322/src/kudu/util/memory/arena.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/memory/arena.h b/src/kudu/util/memory/arena.h
index 18c304d..1c98564 100644
--- a/src/kudu/util/memory/arena.h
+++ b/src/kudu/util/memory/arena.h
@@ -192,10 +192,6 @@ class ArenaBase {
const size_t max_buffer_size_;
size_t arena_footprint_;
- // True if this Arena has already emitted a warning about surpassing
- // the global warning size threshold.
- bool warned_;
-
// Lock covering 'slow path' allocation, when new components are
// allocated and added to the arena's list. Also covers any other
// mutation of the component data structure (eg Reset).
[2/2] kudu git commit: KUDU-1963. Avoid NPE and SSLException when a
TabletClient is closed during negotiation
Posted by jd...@apache.org.
KUDU-1963. Avoid NPE and SSLException when a TabletClient is closed during negotiation
This fixes two related bugs which were triggered when a client called
close() on a TabletClient (e.g. because the end user of the API called
KuduClient#close()):
1) javax.net.ssl.SSLException: renegotiation attempted by peer; closing the connection
This was caused by the following interleaving in Netty:
T1 T2
------------------------ -----------------
TabletClient.close()
Channel.close()
SslHandler.close()
- starts a TLS "shutdown" message
- underlying SSLEngine is now in a
"closing" state call Channel.write()
call SSLEngine.write()
- gets NEEDS_UNWRAP
result
- interprets this as a
renegotiation and
throws an Exception
- call SSLEngine.unwrap() to get the
TLS message data to send
I wasn't able to write a unit test that triggered this reliably, since
TabletClient is too tightly coupled to ConnectionCache and
AsyncKuduClient. However, I was able to reproduce this reliably on an
Impala cluster by starting 100 threads sending short queries round-robin
to 9 impalads. Previously I saw the exception within a few seconds and
now I've run successfully for many minutes with no errors.
2) NullPointerException following above SSLException
When the above SSLException occurred, it called the
TabletClient#exceptionCaught() method, which called 'cleanup()' to fail
any outstanding RPCs. However, because the exception was being caught
because the client had already been explicitly closed, the
'rpcsInflight' and 'pendingRpcs' lists had already been set to null.
This caused an NPE as we tried to process those lists for a second time.
Another case which cased a similar NPE was that the NegotiationResult
could come after the close() had been called, in which case
'pendingRpcs' would already be null.
This patch fixes both issues as follows:
- if we've already explicitly closed the client, we ignore SSLExceptions
rather than logging them. Actually avoiding this race seemed to be
extremely complicated, so better to just ignore it.
- properly check whether the pendingRpcs/rpcsInFlight lists were already
null before processing them.
Change-Id: Ide91722446d01acb58e3e181e0d10932e023a043
Reviewed-on: http://gerrit.cloudera.org:8080/6541
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/ac6e24fc
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/ac6e24fc
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/ac6e24fc
Branch: refs/heads/master
Commit: ac6e24fc2299430b5f70146324bef2d2e8a4b8bc
Parents: 2bc2c32
Author: Todd Lipcon <to...@apache.org>
Authored: Mon Apr 3 18:08:00 2017 -0700
Committer: Jean-Daniel Cryans <jd...@apache.org>
Committed: Tue Apr 4 19:45:04 2017 +0000
----------------------------------------------------------------------
.../org/apache/kudu/client/TabletClient.java | 23 ++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/ac6e24fc/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
----------------------------------------------------------------------
diff --git a/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java b/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
index e26e883..24e80f8 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
@@ -35,6 +35,7 @@ import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.locks.ReentrantLock;
import javax.annotation.concurrent.GuardedBy;
+import javax.net.ssl.SSLException;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
@@ -358,7 +359,12 @@ public class TabletClient extends SimpleChannelUpstreamHandler {
}
// Send the queued RPCs after dropping the lock, so we don't end up calling
// their callbacks/errbacks with the lock held.
- sendQueuedRpcs(queuedRpcs);
+ //
+ // queuedRpcs may be null in the case that we disconnected the client just
+ // at the same time as the Negotiator was finishing up.
+ if (queuedRpcs != null) {
+ sendQueuedRpcs(queuedRpcs);
+ }
return;
}
if (!(m instanceof CallResponse)) {
@@ -633,12 +639,14 @@ public class TabletClient extends SimpleChannelUpstreamHandler {
// for negotiation to complete.
if (pendingRpcs != null) {
rpcsToFail.addAll(pendingRpcs);
+ pendingRpcs = null;
}
- pendingRpcs = null;
// Similarly, we need to fail any that were already sent and in-flight.
- rpcsToFail.addAll(rpcsInflight.values());
- rpcsInflight = null;
+ if (rpcsInflight != null) {
+ rpcsToFail.addAll(rpcsInflight.values());
+ rpcsInflight = null;
+ }
} finally {
lock.unlock();
}
@@ -702,6 +710,13 @@ public class TabletClient extends SimpleChannelUpstreamHandler {
if (!closedByClient) {
LOG.info(getPeerUuidLoggingString() + "Lost connection to peer");
}
+ } else if (e instanceof SSLException && closedByClient) {
+ // There's a race in Netty where, when we call Channel.close(), it tries
+ // to send a TLS 'shutdown' message and enters a shutdown state. If another
+ // thread races to send actual data on the channel, then Netty will get a
+ // bit confused that we are trying to send data and misinterpret it as a
+ // renegotiation attempt, and throw an SSLException. So, we just ignore any
+ // SSLException if we've already attempted to close.
} else {
LOG.error(getPeerUuidLoggingString() + "Unexpected exception from downstream on " + c, e);
}