[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
GitHub user franz1981 opened a pull request:

    https://github.com/apache/activemq-artemis/pull/2181

    ARTEMIS-1980 Warn on failed check of table existence should be info

    DB2 metadata checks should erroneously report stale table existence on
    not existing/just deleted table, making the subsequent warning logs
    of failed SELECT COUNT useless and scaring: should be better to let
    them lowered to INFO level

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1980

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/2181.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2181
   
----
commit a33e3536ee3f186a52d44517398cda4daff763de
Author: Francesco Nigro <nigro.fra@...>
Date:   2018-07-12T13:14:37Z

    ARTEMIS-1980 Warn on failed check of table existence should be info
   
    DB2 metadata checks should erroneously report stale table existence on
    not existing/just deleted table, making the subsequent warning logs
    of failed SELECT COUNT useless and scaring: should be better to let
    them lowered to INFO level

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r202032181
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    +                  logger.debug(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               } else {
    +                  logger.infof("Can't verify the initialization of table %s", tableName);
    --- End diff --
   
    I think this warrants a LOGGER with ID?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r202040381
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    +                  logger.debug(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               } else {
    +                  logger.infof("Can't verify the initialization of table %s", tableName);
    --- End diff --
   
    which one? debug or infof or both?
    I think it worths to know that something has happened at least...just don't want to scare users for nothing...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r202041562
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    +                  logger.debug(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               } else {
    +                  logger.infof("Can't verify the initialization of table %s", tableName);
    --- End diff --
   
    info...
   
   
    logger.debug should always be individualized.. .not LOGGER ids for those!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r202046654
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    +                  logger.debug(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               } else {
    +                  logger.infof("Can't verify the initialization of table %s", tableName);
    --- End diff --
   
    uh good point..I haven't thought on it...and trace? I wanted to add more details just if user want...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r202058086
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    +                  logger.debug(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               } else {
    +                  logger.infof("Can't verify the initialization of table %s", tableName);
    --- End diff --
   
    for trace.. it's the same as debug.. although you usually put on if (log.isTraceEnabled()) in front of it... as it's usually a lot more verbose.
   
    The Logger for debug and trace should be:
    ```java
    Logger logger = Logger.getLogger(somethingThatGetsTheClassAndIdon'trememberNow)
    if (logger.istTraceEnabled()) {
       logger.trace(....);
    }
    ```


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r220121826
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    +                  logger.debug(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               } else {
    +                  logger.infof("Can't verify the initialization of table %s", tableName);
    --- End diff --
   
    I have missed what I should fix on this PR...it is providing a detailed log when DEBUG level is enabled, otherwise will just provide an info. That's because tends DB2 driver to scare users of weird errors when they are not errors at all...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r221229493
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    +                  logger.debug(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               } else {
    +                  logger.infof("Can't verify the initialization of table %s", tableName);
    --- End diff --
   
    @clebertsuconic There is anything I need to do in order to get it merged mate?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r221341393
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    --- End diff --
   
    I feel really wrong on if (logger.isDebug()) logger.debug(...);
    else logger.info(...);
   
   
    it's either always info, or always debug...


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r221341953
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    --- End diff --
   
    the if (logger.isDebugEnable()) is meant to avoid sending something to debug and have it ignored.
   
   
    you have two options:
   
    either:
    ```java
    if (logger.isDebugEnabled()) {
      logger.debug(....);
    }
    ```
   
   
    or
   
   
    ```java
     logger.infof (...);
    ```
   
   
    keep it simple... mixing it like that may confuse debugging and the user In my experience.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2181#discussion_r222663365
 
    --- Diff: artemis-jdbc-store/src/main/java/org/apache/activemq/artemis/jdbc/store/drivers/AbstractJDBCDriver.java ---
    @@ -227,7 +227,11 @@ private void createTableIfNotExists(String tableName, String... sqls) throws SQL
                       }
                    }
                 } catch (SQLException e) {
    -               logger.warn(JDBCUtils.appendSQLExceptionDetails(new StringBuilder("Can't verify the initialization of table ").append(tableName).append(" due to:"), e, sqlProvider.getCountJournalRecordsSQL()));
    +               if (logger.isDebugEnabled()) {
    --- End diff --
   
    I have addressed it maintaining the relevant information about the error only on trace level :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2181: ARTEMIS-1980 Warn on failed check of table exi...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2181
 
    @clebertsuconic  If now it looks fine I would cherry-pick it into 2.6-x too, fater being merged :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #2181: ARTEMIS-1980 Warn on failed check of table exi...

jbertram-2
In reply to this post by jbertram-2
Github user franz1981 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2181
 
    @clebertsuconic @jbertram Guys if is good is possible to merge it? :+1:


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2181: ARTEMIS-1980 Warn on failed check of ta...

jbertram-2
In reply to this post by jbertram-2
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/2181


---