[jira] [Created] (HBASE-25937) Clarify UnknownRegionException
David Mollitor created HBASE-25937: -- Summary: Clarify UnknownRegionException Key: HBASE-25937 URL: https://issues.apache.org/jira/browse/HBASE-25937 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor UnknownRegionException seems to accept a "region name" but it's actually a normal Exception message (and is used that way). Fix this to be "message" and add a "cause" capability as well. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (HBASE-25936) Improve Logging Statement Formatting
David Mollitor created HBASE-25936:
--
Summary: Improve Logging Statement Formatting
Key: HBASE-25936
URL: https://issues.apache.org/jira/browse/HBASE-25936
Project: HBase
Issue Type: Improvement
Reporter: David Mollitor
Assignee: David Mollitor
I reviewed most of the TRACE/DEBUG log statements and wanted to clean up the
first tier issues:
* Use logging anchors {{{}}} where previously was concatenating (without
logging guard)
* Remove String.format and use logging anchors {{{}}}
* Remove logging guards around statements which use anchors (kind of defeats
the purpose of anchors)
* Remove logging guards around log statements which have no parameters (not
concatenation)
* Remove explicit calls to toString() that defeat the purpose of anchors
* Remove Objects.toString() - logging framework will handle {{null}} values
* Remove StringUtils ExceptionStackTrace logging - logging framework will
handle pretty-print of Exception messages and stack traces
* Remove logging guards around INFO messages. They will just about always be
printed in production systems, no need to try to protect them from happening
The goal here is, maybe a tiny bump in performance, but to reduce the size of
the code and to make logging statement more clear to developers.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (HBASE-23628) Remove Apache Commons Digest Base64
David Mollitor created HBASE-23628: -- Summary: Remove Apache Commons Digest Base64 Key: HBASE-23628 URL: https://issues.apache.org/jira/browse/HBASE-23628 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor Use the native JDK Base64 implementation instead. Most places are using the JDK version, but a couple of spots were missed. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (HBASE-23568) Improve Threading of Replication
David Mollitor created HBASE-23568:
--
Summary: Improve Threading of Replication
Key: HBASE-23568
URL: https://issues.apache.org/jira/browse/HBASE-23568
Project: HBase
Issue Type: Improvement
Reporter: David Mollitor
Assignee: David Mollitor
* Use a JDK ExecutorService instead of raw threads
* Untangle the dependencies of the threads with the parent class. Replace
threads with Callables so that a return value can be evaluated by the parent
class and the parent can react accordingly.
* Simplify the 'terminate' routine to use the clean facilities offered by the
{{ExecutorService}}
* Fix some checkstyle failures
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (HBASE-23563) Deprecate Threads shutdown Methods
David Mollitor created HBASE-23563:
--
Summary: Deprecate Threads shutdown Methods
Key: HBASE-23563
URL: https://issues.apache.org/jira/browse/HBASE-23563
Project: HBase
Issue Type: Improvement
Reporter: David Mollitor
{code:java|title=Threads.java}
/**
* Shutdown passed thread using isAlive and join.
* @param t Thread to shutdown
*/
public static void shutdown(final Thread t) {
shutdown(t, 0);
}
/**
* Shutdown passed thread using isAlive and join.
* @param joinwait Pass 0 if we're to wait forever.
* @param t Thread to shutdown
*/
public static void shutdown(final Thread t, final long joinwait) {
if (t == null) return;
while (t.isAlive()) {
try {
t.join(joinwait);
} catch (InterruptedException e) {
LOG.warn(t.getName() + "; joinwait=" + joinwait, e);
}
}
}
{code}
# This method does not actually 'shutdown' anything, it just waits for thread
to die uninterruptedly
# The InterruptedException is lost here
Please deprecated in favor of Guava's {{Uninterruptibles#joinUninterruptibly}}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (HBASE-23556) Minor ChoreServer Cleanup
David Mollitor created HBASE-23556: -- Summary: Minor ChoreServer Cleanup Key: HBASE-23556 URL: https://issues.apache.org/jira/browse/HBASE-23556 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor -- This message was sent by Atlassian Jira (v8.3.4#803005)
Re: Error Handling and Logging
Pedro,
Yes. It is almost entirely subjective. I'm just trying to share with you
my personal experiences of having to administer and troubleshoot these
projects (and all Java projects in particular). Please share some of your
experiences in working with the projects as it relates to logging. I'd
greatly appreciate the feedback.
Thanks!
On Fri, Dec 6, 2019 at 1:25 PM Pedro Boado wrote:
> Wow that's one of the most subjective opinions I've read in a very long
> time.
>
> On Fri, 6 Dec 2019, 17:52 David Mollitor, wrote:
>
> > Hell Team,
> >
> > I'm not sure how to automate this, but as I scan through the log
> > statements, another thing I am observing is the following:
> >
> > 1// Lots of punctuation. There is no need to yell (!!!) or to try to be
> > proper (.). Please be concise and most log messages do not end with a
> > period
> > 2// Contractions. Please do not use contractions (can't, won't we're)
> > these are harder words for our international friends
> > 3// Use strong words. Avoid phases like "couldn't complete the action"
> and
> > instead consider using something like "failed to complete action". The
> > word "fail" is trained into me much more than the word "couldn't"
> >
> > Thanks so much for all the positive feedback I've received from the HBase
> > team here and in private conversations.
> >
> >
> >
> > On Thu, Dec 5, 2019 at 1:10 PM Nick Dimiduk wrote:
> >
> > > I think these are good points all around. Can any of these
> anti-patterns
> > be
> > > flagged by a checkstyle rule? Static analysis would make the
> infractions
> > > easier to track down.
> > >
> > > One more point of my own: I’m of the opinion that we log too much in
> > > general. Info level should not describe the details of operations as
> > > normal. I’m also not a fan of logging data structure “status”messages,
> as
> > > we do, for example, from the block cache. It’s enough to expose these
> as
> > > metrics.
> > >
> > > Thanks for speaking up! If you’re feeling ambitious, please ping me on
> > any
> > > PRs and we’ll get things cleaned up.
> > >
> > > Thanks,
> > > Nick
> > >
> > > On Tue, Dec 3, 2019 at 21:02 Stack wrote:
> > >
> > > > Thanks for the helpful note David. Appreciated.
> > > > S
> > > >
> > > > On Tue, Nov 26, 2019 at 1:44 PM David Mollitor
> > > wrote:
> > > >
> > > > > Hello Team,
> > > > >
> > > > > I am one of many people responsible for supporting the Hadoop
> > products
> > > > out
> > > > > in the field. Error handling and logging are crucial to my
> success.
> > > > I've
> > > > > been reading over the code and I see many of the same mistakes
> again
> > > and
> > > > > again. I just wanted to bring some of these things to your
> attention
> > > so
> > > > > that moving forward, we can make these products better.
> > > > >
> > > > > The general best-practice is:
> > > > >
> > > > > public class TestExceptionLogging
> > > > > {
> > > > > private static final Logger LOG =
> > > > > LoggerFactory.getLogger(TestExceptionLogging.class);
> > > > >
> > > > > public static void main(String[] args) {
> > > > > try {
> > > > > processData();
> > > > > } catch (Exception e) {
> > > > > LOG.error("Application failed", e);
> > > > > }
> > > > > }
> > > > >
> > > > > public static void processData() throws Exception {
> > > > > try {
> > > > > readData();
> > > > > } catch (Exception e) {
> > > > > throw new Exception("Failed to process data", e);
> > > > > }
> > > > > }
> > > > >
> > > > > public static byte[] readData() throws Exception {
> > > > > throw new IOException("Failed to read device");
> > > > > }
> > > > > }
> > > > >
> > > > > Produces:
> > > > >
> > > > > [main] ERROR TestExceptionLogging - Application failed
> > > > > java.lang.Exception: Failed to process data
> > > > > at Test
[jira] [Created] (HBASE-23382) Clean up Logging for Some of hbase-server package
David Mollitor created HBASE-23382: -- Summary: Clean up Logging for Some of hbase-server package Key: HBASE-23382 URL: https://issues.apache.org/jira/browse/HBASE-23382 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor -- This message was sent by Atlassian Jira (v8.3.4#803005)
Re: Error Handling and Logging
Hell Team,
I'm not sure how to automate this, but as I scan through the log
statements, another thing I am observing is the following:
1// Lots of punctuation. There is no need to yell (!!!) or to try to be
proper (.). Please be concise and most log messages do not end with a
period
2// Contractions. Please do not use contractions (can't, won't we're)
these are harder words for our international friends
3// Use strong words. Avoid phases like "couldn't complete the action" and
instead consider using something like "failed to complete action". The
word "fail" is trained into me much more than the word "couldn't"
Thanks so much for all the positive feedback I've received from the HBase
team here and in private conversations.
On Thu, Dec 5, 2019 at 1:10 PM Nick Dimiduk wrote:
> I think these are good points all around. Can any of these anti-patterns be
> flagged by a checkstyle rule? Static analysis would make the infractions
> easier to track down.
>
> One more point of my own: I’m of the opinion that we log too much in
> general. Info level should not describe the details of operations as
> normal. I’m also not a fan of logging data structure “status”messages, as
> we do, for example, from the block cache. It’s enough to expose these as
> metrics.
>
> Thanks for speaking up! If you’re feeling ambitious, please ping me on any
> PRs and we’ll get things cleaned up.
>
> Thanks,
> Nick
>
> On Tue, Dec 3, 2019 at 21:02 Stack wrote:
>
> > Thanks for the helpful note David. Appreciated.
> > S
> >
> > On Tue, Nov 26, 2019 at 1:44 PM David Mollitor
> wrote:
> >
> > > Hello Team,
> > >
> > > I am one of many people responsible for supporting the Hadoop products
> > out
> > > in the field. Error handling and logging are crucial to my success.
> > I've
> > > been reading over the code and I see many of the same mistakes again
> and
> > > again. I just wanted to bring some of these things to your attention
> so
> > > that moving forward, we can make these products better.
> > >
> > > The general best-practice is:
> > >
> > > public class TestExceptionLogging
> > > {
> > > private static final Logger LOG =
> > > LoggerFactory.getLogger(TestExceptionLogging.class);
> > >
> > > public static void main(String[] args) {
> > > try {
> > > processData();
> > > } catch (Exception e) {
> > > LOG.error("Application failed", e);
> > > }
> > > }
> > >
> > > public static void processData() throws Exception {
> > > try {
> > > readData();
> > > } catch (Exception e) {
> > > throw new Exception("Failed to process data", e);
> > > }
> > > }
> > >
> > > public static byte[] readData() throws Exception {
> > > throw new IOException("Failed to read device");
> > > }
> > > }
> > >
> > > Produces:
> > >
> > > [main] ERROR TestExceptionLogging - Application failed
> > > java.lang.Exception: Failed to process data
> > > at TestExceptionLogging.processData(TestExceptionLogging.java:22)
> > > at TestExceptionLogging.main(TestExceptionLogging.java:12)
> > > Caused by: java.io.IOException: Failed to read device
> > > at TestExceptionLogging.readData(TestExceptionLogging.java:27)
> > > at TestExceptionLogging.processData(TestExceptionLogging.java:20)
> > > ... 1 more
> > >
> > >
> > >
> > > Please notice that when an exception is thrown, and caught, it is
> wrapped
> > > at each level and each level adds some more context to describe what
> was
> > > happening when the error occurred. It also produces a complete stack
> > trace
> > > that pinpoints the issue. For Hive folks, it is rarely the case that a
> > > method consuming a HMS API call should itself throw a MetaException.
> The
> > > MetaException has no way of wrapping an underlying Exception and
> helpful
> > > data is often loss. A method may chooses to wrap a MetaException, but
> it
> > > should not be throwing them around as the default behavior.
> > >
> > > Also important to note is that there is exactly one place that is doing
> > the
> > > logging. There does not need to be any logging at the lower levels. A
> > > catch block should throw or log, not both. This is an anti-pattern and
> > > annoying as the end user: having to deal with multiple stack
[jira] [Created] (HBASE-23381) Improve Logging in HBase Commons Package
David Mollitor created HBASE-23381: -- Summary: Improve Logging in HBase Commons Package Key: HBASE-23381 URL: https://issues.apache.org/jira/browse/HBASE-23381 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor Based on my observations and suggestions here: https://lists.apache.org/thread.html/71f546c89ecdaa2f25a26bd238e88680ddaad1d3b5c4031338d3533a%40%3Cdev.hbase.apache.org%3E -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (HBASE-23380) General Cleanup of FSUtil
David Mollitor created HBASE-23380: -- Summary: General Cleanup of FSUtil Key: HBASE-23380 URL: https://issues.apache.org/jira/browse/HBASE-23380 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor * Clean up JavaDocs * Clean up logging and error messages * Remove superfluous code * Replace static code with library call * Do not swallow Interrupted Exceptions * Use try-with-resources * User multi-Exception catches to reduce code size -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (HBASE-23379) Clean Up FSUtil getRegionLocalityMappingFromFS
David Mollitor created HBASE-23379:
--
Summary: Clean Up FSUtil getRegionLocalityMappingFromFS
Key: HBASE-23379
URL: https://issues.apache.org/jira/browse/HBASE-23379
Project: HBase
Issue Type: Improvement
Reporter: David Mollitor
Assignee: David Mollitor
* Use Java's {{Executors}} convenience class
* Do not swallow InterruptedException's state
* Passing superfluous 'null' value through the call stack (remove it)
* General cleanup
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (HBASE-23378) Clean Up FSUtil setClusterId
David Mollitor created HBASE-23378: -- Summary: Clean Up FSUtil setClusterId Key: HBASE-23378 URL: https://issues.apache.org/jira/browse/HBASE-23378 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor * Use try-with-resources * Remove bad practice of catching one's own Exceptions * Method signature 'wait' should be of type long to match JDK API * Add additional debugging * Do not swallow Interrupt status of thread * General cleanup -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (HBASE-23368) Optimize Debug Logging of ClientZKSyncer
David Mollitor created HBASE-23368:
--
Summary: Optimize Debug Logging of ClientZKSyncer
Key: HBASE-23368
URL: https://issues.apache.org/jira/browse/HBASE-23368
Project: HBase
Issue Type: Improvement
Reporter: David Mollitor
Assignee: David Mollitor
In particular, this caught my eye:
{code:java|title=ClientZKSyncer.java}
// set meta znodes for client ZK
Collection nodes = getNodesToWatch();
LOG.debug("Znodes to watch: " + nodes);
{code}
There are no guards around this debug statement, so the entire list of nodes is
always turned into a string and passed to the {{debug}} method,... and then
promptly discarded if debug is not enabled.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
Error Handling and Logging
Hello Team,
I am one of many people responsible for supporting the Hadoop products out
in the field. Error handling and logging are crucial to my success. I've
been reading over the code and I see many of the same mistakes again and
again. I just wanted to bring some of these things to your attention so
that moving forward, we can make these products better.
The general best-practice is:
public class TestExceptionLogging
{
private static final Logger LOG =
LoggerFactory.getLogger(TestExceptionLogging.class);
public static void main(String[] args) {
try {
processData();
} catch (Exception e) {
LOG.error("Application failed", e);
}
}
public static void processData() throws Exception {
try {
readData();
} catch (Exception e) {
throw new Exception("Failed to process data", e);
}
}
public static byte[] readData() throws Exception {
throw new IOException("Failed to read device");
}
}
Produces:
[main] ERROR TestExceptionLogging - Application failed
java.lang.Exception: Failed to process data
at TestExceptionLogging.processData(TestExceptionLogging.java:22)
at TestExceptionLogging.main(TestExceptionLogging.java:12)
Caused by: java.io.IOException: Failed to read device
at TestExceptionLogging.readData(TestExceptionLogging.java:27)
at TestExceptionLogging.processData(TestExceptionLogging.java:20)
... 1 more
Please notice that when an exception is thrown, and caught, it is wrapped
at each level and each level adds some more context to describe what was
happening when the error occurred. It also produces a complete stack trace
that pinpoints the issue. For Hive folks, it is rarely the case that a
method consuming a HMS API call should itself throw a MetaException. The
MetaException has no way of wrapping an underlying Exception and helpful
data is often loss. A method may chooses to wrap a MetaException, but it
should not be throwing them around as the default behavior.
Also important to note is that there is exactly one place that is doing the
logging. There does not need to be any logging at the lower levels. A
catch block should throw or log, not both. This is an anti-pattern and
annoying as the end user: having to deal with multiple stack traces at
different log levels for the same error condition. The log message should
be at the highest level only.
https://community.oracle.com/docs/DOC-983543#logAndThrow
Both projects use SLF4J as the logging framework (facade anyway). Please
familiarize yourself with how to correctly log an Exception. There is no
need to log a thread name, a time stamp, a class name, or a stack trace.
The logging framework will do that all for you.
http://www.slf4j.org/faq.html#paramException
Again, there is no need to 'stringify' an exception. For example, do not
use this:
https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java#L86
If you do however want to dump a stack trace for debugging (or trace)
purposes, consider performing the following:
if (LOG.isDebugEnabled()) {
LOG.debug("Dump Thread Stack", new Exception("Thread Stack Trace (Not an
Error)"));
}
Finally, I've seen it a couple of times in Apache project that enabling
debug-level logging causes the application to emit logs at other levels,
for example:
LOG.warn("Some error occurred: {}", e.getMessage());
if (LOG.isDebugEnabled()) {
LOG. warn("Dump Warning Thread Stack", e);
}
Please refrain from doing this. The inner log statement should be at DEBUG
level to match the check. Otherwise, when I enable DEBUG logging in the
application, the expectation that I have is that I will have the exact
logging as the INFO level, but I will also have additional DEBUG details as
well. I am going to be using 'grep' to find DEBUG and I will miss this
additional logging. I will then be peeved when I say "ah ha! It's a WARN
log message, I don't need DEBUG enabled to get this logging when the issue
happens again!"... but then I restart the application and discover I do in
fact need DEBUG logging enabled.
Finally, finally, do not pass server-generated stack traces to a client
application. When a client dumps a stack trace, it is not always trivial
to decide if the stack trace is in regards to the client or the server.
This can also be a security issue as it may hint to the client which
libraries and (based on line numbers) which versions of the libraries are
being used. If the server is going to generate an error in response to a
client API call, the server should log the exception (right before
returning the response to the client) and it should only pass a helpful
error message to the client. The operator/administrator can look in the
server logs for the details.
Thanks!
[jira] [Created] (HBASE-23308) Review of NullPointerExceptions
David Mollitor created HBASE-23308: -- Summary: Review of NullPointerExceptions Key: HBASE-23308 URL: https://issues.apache.org/jira/browse/HBASE-23308 Project: HBase Issue Type: Improvement Reporter: David Mollitor Assignee: David Mollitor -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Created] (HBASE-23102) Improper Usage of Map putIfAbsent
David Mollitor created HBASE-23102:
--
Summary: Improper Usage of Map putIfAbsent
Key: HBASE-23102
URL: https://issues.apache.org/jira/browse/HBASE-23102
Project: HBase
Issue Type: Improvement
Affects Versions: 2.3.0
Reporter: David Mollitor
When using {{Map#putIfAbsent}}, the argument should not be a {{new}} object.
Otherwise, if the item is present, the object that was instantiated is
immediately thrown away. Instead, use {{Map#computeIfAbsent}} so that the
object is only instantiated if it is needed.
There exists a good example in the {{Map}} JavaDoc:
https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-
h2. Locations
https://github.com/apache/hbase/blob/9370347efea5b09e2fa8f4e5d82fa32491e1181b/hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/QuotaCache.java#L227-L236
https://github.com/apache/hbase/blob/025ddce868eb06b4072b5152c5ffae5a01e7ae30/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/throttle/StoreHotnessProtector.java#L124-L129
https://github.com/apache/hbase/blob/1170f28122d9d36e511ba504a5263ec62e11ef6a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java#L555
https://github.com/apache/hbase/blob/4ca760fe9dd373b8d8a4c48db15e42424920653c/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java#L584-L586
https://github.com/apache/hbase/blob/4ca760fe9dd373b8d8a4c48db15e42424920653c/hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java#L585
https://github.com/apache/hbase/blob/5b01e613fbbb92e243e99a1d199b4ffbb21ed2d9/hbase-procedure/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureExecutor.java#L834
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (HBASE-22947) Client Should Prompt For Additional Confirmation on System Table DDL
David Mollitor created HBASE-22947: -- Summary: Client Should Prompt For Additional Confirmation on System Table DDL Key: HBASE-22947 URL: https://issues.apache.org/jira/browse/HBASE-22947 Project: HBase Issue Type: Improvement Components: Client Affects Versions: 2.0.0, 2.1.0 Reporter: David Mollitor If a user is going to perform a DDL/disable operation on a system table, the client should print a warning to the screen warning of the risks and prompt something like "Are you sure?" -- This message was sent by Atlassian Jira (v8.3.2#803003)
[jira] [Created] (HBASE-22604) Dead Link in Docs
David Mollitor created HBASE-22604: -- Summary: Dead Link in Docs Key: HBASE-22604 URL: https://issues.apache.org/jira/browse/HBASE-22604 Project: HBase Issue Type: Improvement Components: documentation Reporter: David Mollitor _blog post Understanding HBase and BigTable by Jim R. Wilson_ Link is dead. Please update or remove. https://hbase.apache.org/book.html#conceptual.view -- This message was sent by Atlassian JIRA (v7.6.3#76005)
HBase Compressed Data in ZNodes
Hello Team, ZooKeeper recently added a feature to enable compression for values stored in ZNodes when the nodes are persisted to disk. Obviously this feature is less interesting if the data written into the ZNodes is already compressed. https://issues.apache.org/jira/browse/ZOOKEEPER-3179 With that said, I know HBase is a big consumer of ZNode services. Does HBase store any sizable information in ZNodes? If so, is this data compressed? Would HBase installations gain from this new feature? Thanks!
