[
https://issues.apache.org/jira/browse/CASSANDRA-6656?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13893701#comment-13893701
]
Mikhail Stepura commented on CASSANDRA-6656:
--------------------------------------------
[~d.yuan]
Thanks, but I still have a couple of comments for *DynamicCompositeType*
{code}
catch (CharacterCodingException ce)
{
// ByteBufferUtil.string failed.
// Log it here and we'll further throw an exception below since
comparator == null
logger.error("Failed when converting value to string. ", ce);
}
catch (Exception e)
{
// parse failed.
// Log it here and we'll further throw an exception below since
comparator == null
logger.error("Failed to parse value string: {}. Exception
follows. ", valueStr, e);
}
{code}
* The first {{logger.error()}} at CharacterCodingException will print out the
entire stack for the exception, because it's {{Logger.error(String,
Throwable)}} . You only print a message in all other cases. Why the full stack
trace here?
* The second one (at Exception) will not print exception at all. You are using
{{Logger.error(String, Object, Object)}} but provide only 1 formatting anchor
({}) for {{valueStr}} so exception will not be logged at all.
> Exception logging
> -----------------
>
> Key: CASSANDRA-6656
> URL: https://issues.apache.org/jira/browse/CASSANDRA-6656
> Project: Cassandra
> Issue Type: Improvement
> Components: Core, Tools
> Reporter: Ding Yuan
> Assignee: Ding Yuan
> Priority: Trivial
> Fix For: 2.1
>
> Attachments: trunk-6656-v2.txt, trunk-6656.txt
>
>
> Reporting a few cases where informative exceptions can be silently swallowed.
> Attaching a proposed patch.
> =========================
> Case 1
> Line: 95, File: "org/apache/cassandra/utils/Hex.java"
> An actual failure in the underlying constructor will be lost.
> Propose to log it.
> {noformat}
> try
> {
> s = stringConstructor.newInstance(0, c.length, c);
> + }
> + catch (InvocationTargetException ite) {
> + // The underlying constructor failed. Unwrapping the
> exception.
> + logger.info("Underlying constructor throws exception: ",
> ite.getCause());
> }
> catch (Exception e)
> {
> // Swallowing as we'll just use a copying constructor
> }
> return s == null ? new String(c) : s;
> {noformat}
> ==========================================
> =========================
> Case 2
> Line: 192, File: "org/apache/cassandra/db/marshal/DynamicCompositeType.java"
> The actual cause of comparator error can be lost as it can fail in multiple
> locations.
> {noformat}
> AbstractType<?> comparator = null;
> int header = getShortLength(bb);
> if ((header & 0x8000) == 0)
> {
> ByteBuffer value = getBytes(bb, header);
> try
> {
> comparator = TypeParser.parse(ByteBufferUtil.string(value));
> }
> catch (Exception e)
> {
> <--- can fail here
> // we'll deal with this below since comparator == null
> }
> }
> else
> {
> comparator = aliases.get((byte)(header & 0xFF));
> <--- can fail here
> }
> if (comparator == null)
> throw new MarshalException("Cannot find comparator for component
> " + i);
> {noformat}
> Propose to log the exception.
> ==========================================
> =========================
> Case 3
> Line: 239, File: "org/apache/cassandra/tools/NodeProbe.java"
> Exception ignored in finally. Propose log them with debug or trace.
> {noformat}
> 232: finally
> 233: {
> 234: try
> 235: {
> 236: ssProxy.removeNotificationListener(runner);
> 236: ssProxy.removeNotificationListener(runner);
> 237: jmxc.removeConnectionNotificationListener(runner);
> 238: }
> 239: catch (Throwable ignored) {}
> 240: }
> {noformat}
> Similar case is at line 264 in the same file.
> ==========================================
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
