[GitHub] zookeeper pull request #669: ZOOKEEPER-3152: Port ZK netty stack to netty4

2018-10-15 Thread dain
Github user dain commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/669#discussion_r225332620
  
--- Diff: 
zookeeper-common/src/test/java/org/apache/zookeeper/common/TestByteBufAllocator.java
 ---
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.CompositeByteBuf;
+import io.netty.buffer.PooledByteBufAllocator;
+import io.netty.util.ResourceLeakDetector;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * This is a custom ByteBufAllocator that tracks outstanding allocations 
and
+ * crashes the program if any of them are leaked.
+ *
+ * Never use this class in production, it will cause your server to run out
+ * of memory! This is because it holds strong references to all allocated
+ * buffers and doesn't release them until checkForLeaks() is called at the
+ * end of a unit test.
+ *
+ * Note: the original code was copied from 
https://github.com/airlift/drift,
+ * with the permission and encouragement of airlift's author (dain). 
Airlift
+ * uses the same apache 2.0 license as Zookeeper so this should be ok.
+ *
+ * However, the code was modified to take advantage of Netty's built-in
+ * leak tracking and make a best effort to print details about buffer 
leaks.
+ */
+public class TestByteBufAllocator extends PooledByteBufAllocator {
+private static AtomicReference INSTANCE =
+new AtomicReference<>(null);
+
+/**
+ * Get the singleton testing allocator.
+ * @return the singleton allocator, creating it if one does not exist.
+ */
+public static TestByteBufAllocator getInstance() {
+TestByteBufAllocator result = INSTANCE.get();
+if (result == null) {
+// Note: the leak detector level never gets reset after this,
+// but that's probably ok since this is only used by test code.
+
ResourceLeakDetector.setLevel(ResourceLeakDetector.Level.PARANOID);
+INSTANCE.compareAndSet(null, new TestByteBufAllocator());
+result = INSTANCE.get();
+}
+return result;
+}
+
+/**
+ * Destroys the singleton testing allocator and throws an error if any 
of the
+ * buffers allocated by it have been leaked. Attempts to print leak 
details to
+ * standard error before throwing, by using netty's built-in leak 
tracking.
+ * Note that this might not always work, since it only triggers when a 
buffer
+ * is garbage-collected and calling System.gc() does not guarantee 
that a buffer
+ * will actually be GC'ed.
+ *
+ * This should be called at the end of a unit test's tearDown() method.
+ */
+public static void checkForLeaks() {
+TestByteBufAllocator result = INSTANCE.getAndSet(null);
+if (result != null) {
+result.checkInstanceForLeaks();
+}
+
+}
+
+private final List trackedBuffers = new ArrayList<>();
+
+public TestByteBufAllocator()
+{
+super(false);
+}
+
+@Override
+protected ByteBuf newHeapBuffer(int initialCapacity, int maxCapacity)
+{
+return track(super.newHeapBuffer(initialCapacity, maxCapacity));
+}
+
+@Override
+protected ByteBuf newDirectBuffer(int initialCapacity, int maxCapacity)
+{
+return track(super.newDirectBuffer(initialCapacity, maxCapacity));
+}
+
+@Override
+public CompositeByteBuf compositeHeapBuffer(int maxNumComponents)
+{
+return track(super.compositeHeapBuffer(maxNumComponents));
+}
+
+@Override
+public CompositeByteBuf compositeD

[GitHub] zookeeper pull request #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast...

2018-06-14 Thread dain
Github user dain commented on a diff in the pull request:

https://github.com/apache/zookeeper/pull/184#discussion_r195534358
  
--- Diff: src/java/main/org/apache/zookeeper/common/X509Util.java ---
@@ -79,7 +91,7 @@
 public X509Util() {
 String cipherSuitesInput = 
System.getProperty(cipherSuitesProperty);
 if (cipherSuitesInput == null) {
-cipherSuites = null;
+cipherSuites = getDefaultCipherSuites();
--- End diff --

I would say that if a user manually selected a cipher suite and it is not 
present on the JVM it should be an error.  Otherwise they are not getting what 
they explicitly asked for.  That said, I'm not sure you can check this 
explicitly (I don't know the apis that well either... especially when multiple 
providers are installed).


---