Alissa Bonas has posted comments on this change. Change subject: core: Locate data source in a loop ......................................................................
Patch Set 1: (2 inline comments) .................................................... File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacadeLocator.java Line 51: "check it. Will try again in a few seconds."); Line 52: Line 53: // Wait a bit before trying again: Line 54: Thread.sleep(DEFAULT_INTERVAL_VALUE); Line 55: } Juan, indeed the static block was here before this patch - but adding an endless loop and a sleep to it makes it worse. The patch (and code in general) is judged on several parameters among which are stability , how easy the code is maintainable and whether it can be troubleshooted in timely manner and whenever is possible without debugging (imagine customer environments to which you are not allowed to connect in order to debug). And static block with vast logic doesn't do good to any of the measurements I mentioned. And you indeed raise a very important point - as long as everything works as expected - it's all good. But when it doesn't work as expected - troubleshooting what caused NoClassDefFoundError is really not trivial, troubleshooting endless loop is no picnic as well, and when we write code, we should write it the way that it also be possible and easy to troubleshoot when things go wrong. If the datasource should be initialized before the application is started (which makes good sense :)) , then it should be configured as a dependency on application server level. If I remember correctly, at least in previous versions of JBoss there was such option. Line 56: Line 57: // Create the facade and return it: Line 58: dbFacade = new DbFacade(); Line 59: dbFacade.setDbEngineDialect(loadDbEngineDialect()); Line 51: "check it. Will try again in a few seconds."); Line 52: Line 53: // Wait a bit before trying again: Line 54: Thread.sleep(DEFAULT_INTERVAL_VALUE); Line 55: } Roy, I agree with you. Datasource is a configuration that application server initializes , and if it fails to init, the application should not load. There should be a dependency on app server level to enforce correct order of initialization and deployment. Checking for that in static block inside some internal class of the web application is not the right place. Line 56: Line 57: // Create the facade and return it: Line 58: dbFacade = new DbFacade(); Line 59: dbFacade.setDbEngineDialect(loadDbEngineDialect()); -- To view, visit http://gerrit.ovirt.org/10189 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I72c99c61d05e8a1619c7d1fb70af956d1050eb3a Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
