Mongeez fail-fast behavior #35

Open
wants to merge 3 commits into from

2 participants

@dtserekhman
  • As per Google Groups discussion with "davidmc24" (see below), introduced MongeezException and started throwing it instead of logger.error(..) logging for fail-fast behavior. There was also at least one usage of logger.warn(..) that I replaced with MongeezException. Also, I simplified and used "catch-all" Exception to re-throw it as MongeezException.

davidmc24

"I think that it may have been me that introduced this behavior. My main goal at the time was for the output to be consistently logged, rather than go to System.out/err. I'd be fine with petty much all scenarios failing fast (no config option needed). Maybe introduce a MongeezException class and throw that in the catch blocks?"

Sent from Mailbox for iPhone

On Tue, Nov 19, 2013 at 6:18 PM, Dmitriy Tserehman dmitr...@gmail.com wrote:

  • show quoted text -
  • show quoted text -
    Was there a thought of either re-throwing exception after logging it or just throwing some Unchecked or other RuntimeException? May be such behavior could be also made conditional, similarly to how it is done with the "failOnError" flag on the ChangeSet?

    } catch (RuntimeException e) {
        if (changeSet.isFailOnError()) {
            throw e;
        } else {
            logger.warn("ChangeSet " + changeSet.getChangeId() + " has failed, but failOnError is set to false", e.getMessage());
        }
    }
    

Thanks,
Dmitriy.

Dmitriy added some commits Nov 20, 2013
Dmitriy Mongeez fail-fast behavior
+ As per Google Groups discussion with "davidmc24" (see below), introduced MongeezException and started throwing it instead of logger.error(..) logging for fail-fast behavior.

-----------------------------
davidmc24

"I think that it may have been me that introduced this behavior. My main goal at the time was for the output to be consistently logged, rather than go to System.out/err. I'd be fine with petty much all scenarios failing fast (no config option needed). Maybe introduce a MongeezException class and throw that in the catch blocks?"
—
Sent from Mailbox for iPhone

On Tue, Nov 19, 2013 at 6:18 PM, Dmitriy Tserehman <dmitr...@gmail.com> wrote:

- show quoted text -
- show quoted text -
Was there a thought of either re-throwing exception after logging it or just throwing some Unchecked or other RuntimeException? May be such behavior could be also made conditional, similarly to how it is done with the "failOnError" flag on the ChangeSet?

        } catch (RuntimeException e) {
            if (changeSet.isFailOnError()) {
                throw e;
            } else {
                logger.warn("ChangeSet " + changeSet.getChangeId() + " has failed, but failOnError is set to false", e.getMessage());
            }
        }

Thanks,
Dmitriy.
2a5e76f
Dmitriy Deleted log4j.properties file
+ Log4j configuration should be handled outside, by the application
92eecc6
@davidmc24 davidmc24 commented on the diff Nov 21, 2013
src/main/java/org/mongeez/reader/FilesetXMLReader.java
}
- } catch (IOException e) {
- logger.error("IOException", e);
- } catch (org.xml.sax.SAXException e) {
- logger.error("SAXException", e);
+ } catch (Exception e) {

Is there a reason you changed this to catch Exception rather than specific types? The same question applies to other places in the commit.

I was debating on this. Since there was no special handling happening per specific type of Exception, why just not catch all and re-throw it as MongeezException?

The reason I tend not to do that is that it also catches any number of other exceptions that you may not really want to. ClassNotFoundException, NullPointerException, NoSuchMethodException...

If this library were Java 7+, I'd just use multi-catch.

I understand. In my case, I do not see much difference if the program fails with the NullPointerException or MongeezException that wraps around the NullPointerException. But I don't have strong preference here. Do you want me to put IOException and the parse exception catch blocks back in and get rid of the catch-all?

Thanks,
Dmitriy.

@szpak
szpak added a note Jan 13, 2014

Catching all exceptions (under Exception and even worse all Throwable exceptions) is usually treated as a smell in code. MongeezException could suggest that there was some "business" problem while importing data. Using this code in my code I would prefer to get NPE and other generic exceptions directly (not wrapped).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davidmc24 davidmc24 and 1 other commented on an outdated diff Nov 21, 2013
src/main/java/org/mongeez/reader/XmlChangeSetReader.java
@@ -58,7 +57,7 @@ public boolean supports(Resource file) {
logger.info("Parsing XML Change Set File {}", file.getFilename());
ChangeSetList changeFileSet = (ChangeSetList) digester.parse(file.getInputStream());
if (changeFileSet == null) {
- logger.warn("Ignoring change file {}, the parser returned null. Please check your formatting.", file.getFilename());
+ throw new MongeezException("Ignoring change file \"" + file.getFilename() + "\", the parser returned null. Please check your formatting.");

If this is now going to be a fatal condition, we should probably change the wording of the message.

Ah, agree. How about this message?

"Change file \"" + file.getFilename() + "\" could not be parsed. changeFileSet was null. Please check your formatting."

I just added another commit to this pull request where I changed the warn message to the above.

I also fixed one unit test that started failing due to some environment specific path I was asserting on.

Thanks.

Makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Dmitriy Unit test fix and warn message change
+ Fixed unit test failing due to environment specific path

+ Changed one of the warn messages
7f03b0f
@szpak

I have came here to report the same issue, but found this. I use Mongeez from Spring and it would be useful to throw an exception to tell Spring that something is wrong and the whole context cannot be setup successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment