Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JBTM-3273 -> Avoid implementing InvocationHandler in jtaLogger #1590

Merged
merged 1 commit into from
Apr 15, 2020
Merged

JBTM-3273 -> Avoid implementing InvocationHandler in jtaLogger #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions ArjunaJTA/jta/classes/com/arjuna/ats/internal/jta/recovery/arjunacore/XARecoveryModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import com.arjuna.ats.internal.arjuna.common.UidHelper;
import com.arjuna.ats.internal.jta.resources.arjunacore.XAResourceRecord;
import com.arjuna.ats.jta.common.jtaPropertyManager;
import com.arjuna.ats.jta.logging.RecoveryRequired;
import com.arjuna.ats.jta.logging.jtaLogger;
import com.arjuna.ats.jta.recovery.SerializableXAResourceDeserializer;
import com.arjuna.ats.jta.recovery.XARecoveryResource;
Expand Down Expand Up @@ -91,7 +92,7 @@ public Set<String> getContactedJndiNames() {

@Override
public boolean isPeriodicWorkSuccessful() {
return !jtaLogger.isRecoveryProblems();
return !RecoveryRequired.isRecoveryProblems();
}

public void addXAResourceRecoveryHelper(XAResourceRecoveryHelper xaResourceRecoveryHelper) {
Expand Down Expand Up @@ -189,7 +190,7 @@ private synchronized void periodicWorkFirstPass(ScanStates endState)
}

contactedJndiNames.clear();
jtaLogger.setRecoveryProblems(false);
RecoveryRequired.setRecoveryProblems(false);

_uids = new InputObjectState();

Expand All @@ -201,15 +202,18 @@ private synchronized void periodicWorkFirstPass(ScanStates endState)
{
if (!_recoveryStore.allObjUids(_recoveryManagerClass.type(), _uids))
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_alluids();
}
}
catch (ObjectStoreException e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_objstoreerror(e);
}
catch (Exception e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_periodicfirstpass(_logName+".periodicWorkFirstPass", e);
}
// JBTM-1354 JCA needs to be able to recover XAResources associated with a subordinate transaction so we have to do at least
Expand All @@ -225,6 +229,7 @@ private synchronized void periodicWorkFirstPass(ScanStates endState)
try {
xaRecoveryFirstPass(xaResource);
} catch (Exception ex) {
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_getxaresource(ex);
}
}
Expand All @@ -234,6 +239,7 @@ private synchronized void periodicWorkFirstPass(ScanStates endState)
try {
xaResource.recover(XAResource.TMENDRSCAN);
} catch (Exception ex) {
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_getxaresource(ex);
}
}
Expand Down Expand Up @@ -276,6 +282,7 @@ public synchronized void periodicWorkSecondPass()
}
catch (Exception e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_periodicsecondpass(_logName+".periodicWorkSecondPass", e);
}

Expand Down Expand Up @@ -373,6 +380,7 @@ protected XARecoveryModule(XARecoveryResourceManager recoveryClass, String logNa
_logName = logName;
_recoveryManagerClass = recoveryClass;
if(_recoveryManagerClass == null) {
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_constfail();
}

Expand Down Expand Up @@ -432,6 +440,7 @@ record = _recoveryManagerClass.getResource(theUid);
}
else
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_recoveryfailed(theUid, XARecoveryResourceHelper.stringForm(recoveryStatus));
}
}
Expand Down Expand Up @@ -469,6 +478,7 @@ record = _recoveryManagerClass.getResource(theUid);
{
problem = true;

RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_recoveryerror(e);
}

Expand All @@ -485,6 +495,7 @@ record = _recoveryManagerClass.getResource(theUid);

if (record.getXid() == null)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_cannotadd();
}
else
Expand All @@ -501,6 +512,7 @@ record = _recoveryManagerClass.getResource(theUid);
}
catch (Throwable e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_unexpectedrecoveryerror(e);
}
}
Expand All @@ -518,6 +530,7 @@ private void bottomUpRecovery() {
try {
xaRecoverySecondPass(xaResource);
} catch (Exception ex) {
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_getxaresource(ex);
}
}
Expand Down Expand Up @@ -582,6 +595,7 @@ private final List<XAResource> resourceInitiatedRecovery()
}
catch (Exception ex)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_getxaresource(ex);
}
}
Expand Down Expand Up @@ -612,6 +626,7 @@ private List<XAResource> resourceInitiatedRecoveryForRecoveryHelpers()
}
catch (Exception ex)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_getxaresource(ex);
}
}
Expand Down Expand Up @@ -667,6 +682,7 @@ private final void xaRecoveryFirstPass(XAResource xares)
}
catch (XAException e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_xarecovery1(_logName+".xaRecovery", XAHelper.printXAErrorCode(e), e);

try
Expand Down Expand Up @@ -801,6 +817,7 @@ private void xaRecoverySecondPass(XAResource xares) {

if (recoveryStatus != XARecoveryResource.RECOVERED_OK)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_failedtorecover(_logName+".xaRecovery", XARecoveryResourceHelper.stringForm(recoveryStatus));
}

Expand All @@ -815,6 +832,7 @@ private void xaRecoverySecondPass(XAResource xares) {
}
catch (Exception e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_forgetfailed(_logName+".xaRecovery", e);
}
}
Expand All @@ -825,6 +843,7 @@ private void xaRecoverySecondPass(XAResource xares) {
}
catch (Exception e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_generalrecoveryerror(_logName + ".xaRecovery", e);
}

Expand All @@ -835,6 +854,7 @@ private void xaRecoverySecondPass(XAResource xares) {
}
catch (XAException e)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_xarecovery1(_logName+".xaRecovery", XAHelper.printXAErrorCode(e), e);
}
}
Expand Down Expand Up @@ -891,6 +911,7 @@ else if(vote == XAResourceOrphanFilter.Vote.ROLLBACK)
}
else
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_xarecovery1(_logName+".xaRecovery", XAHelper.printXAErrorCode(e1), e1);
}

Expand All @@ -912,6 +933,7 @@ else if(vote == XAResourceOrphanFilter.Vote.ROLLBACK)
}
catch (Exception e2)
{
RecoveryRequired.setRecoveryProblems(true);
jtaLogger.i18NLogger.warn_recovery_xarecovery2(_logName+".xaRecovery", e2);
}
return false;
Expand Down
14 changes: 14 additions & 0 deletions ArjunaJTA/jta/classes/com/arjuna/ats/jta/logging/RecoveryRequired.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.arjuna.ats.jta.logging;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the placing the class under the logging package. As it informs about recovery processing it makes more sense to me to put it under recovery package. What was your reasoning about placing it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this class is only used by jtaLogger for now, therefore I thought of placing it under com.arjuna.ats.jta.logging package would be fine for now. Maybe later if any other package requires it, then we could move it under recovery package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will move it to recovery package


public class RecoveryRequired {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your reasoning for naming the class as RecoveryRequired?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would add meaning to the action, that user has to take after he gets the result.


private static boolean recoveryProblems;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is a flag which is used to mark if a recovery problem occurs and this flag could be accessed from different threads and value could be set from the different threads then the code has to(!) consider that. The property should be of type atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this thought. Thanks for pointing this out. I will make it thread safe.


public static boolean isRecoveryProblems() {
return recoveryProblems;
}

public static void setRecoveryProblems(boolean recoveryProblems) {
RecoveryRequired.recoveryProblems = recoveryProblems;
}
}
39 changes: 7 additions & 32 deletions ArjunaJTA/jta/classes/com/arjuna/ats/jta/logging/jtaLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,47 +33,22 @@

import org.jboss.logging.Logger;

import java.lang.reflect.InvocationHandler;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;

public class jtaLogger implements InvocationHandler
public class jtaLogger
{
public static final Logger logger = Logger.getLogger("com.arjuna.ats.jta");
public static final jtaI18NLogger i18NLogger = (jtaI18NLogger) Proxy.newProxyInstance(
jtaI18NLogger.class.getClassLoader(),
new Class[] { jtaI18NLogger.class },
new jtaLogger(Logger.getMessageLogger(jtaI18NLogger.class, "com.arjuna.ats.jta")));

/**
* For jtaI18NLogger, if any method, prefixed with <b>"warn_recovery"</b> is called then,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good comment but I consider it should be put into the class jtaI18Logger as it's the place where new methods for logging are added (and where the methods are named). I think when it's here it's better chance to be missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will move the comments to jtaI18Logger

* <b>RecoverRequired</b> class variable i.e. "recoveryProblem" should be set as <b>true</b>.
* */
public static final jtaI18NLogger i18NLogger = Logger.getMessageLogger(jtaI18NLogger.class, "com.arjuna.ats.jta");

private jtaI18NLogger jtaI18NLoggerImpl;
private static boolean recoveryProblems;

private jtaLogger() {
}

private jtaLogger(jtaI18NLogger logger) {
jtaI18NLoggerImpl = logger;
}

public static boolean isRecoveryProblems() {
return recoveryProblems;
}

public static void setRecoveryProblems(boolean recoveryProblems) {
jtaLogger.recoveryProblems = recoveryProblems;
}

@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
try {
if (method.getName().startsWith("warn_recovery")) {
recoveryProblems = true;
}

return method.invoke(jtaI18NLoggerImpl, args);
} catch (InvocationTargetException e) {
throw e.getCause() != null ? e.getCause() : e;
}
}
}
3 changes: 2 additions & 1 deletion ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.arjuna.ats.internal.jta.recovery.arjunacore.XARecoveryModule;
import com.arjuna.ats.jta.common.jtaPropertyManager;
import com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple;
import com.arjuna.ats.jta.logging.RecoveryRequired;
import com.arjuna.ats.jta.logging.jtaLogger;
import org.junit.Test;

Expand Down Expand Up @@ -80,7 +81,7 @@ public void testRecoveryMonitorWithFailure() throws Exception {
// check the output of the scan
assertEquals("ERROR", RecoveryMonitor.getResponse());
assertEquals("ERROR", RecoveryMonitor.getSystemOutput());
assertTrue(jtaLogger.isRecoveryProblems());
assertTrue(RecoveryRequired.isRecoveryProblems());
} finally {
manager.terminate();
}
Expand Down
3 changes: 2 additions & 1 deletion ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryMonitorTest2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.arjuna.ats.arjuna.tools.RecoveryMonitor;
import com.arjuna.ats.internal.jta.recovery.arjunacore.XARecoveryModule;
import com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple;
import com.arjuna.ats.jta.logging.RecoveryRequired;
import com.arjuna.ats.jta.logging.jtaLogger;
import org.junit.Test;

Expand Down Expand Up @@ -122,7 +123,7 @@ public void start(Xid xid, int i) throws XAException {
// check the output of the scan
assertEquals("DONE", RecoveryMonitor.getResponse());
assertEquals("DONE", RecoveryMonitor.getSystemOutput());
assertFalse(jtaLogger.isRecoveryProblems());
assertFalse(RecoveryRequired.isRecoveryProblems());
} finally {
manager.terminate();
}
Expand Down
5 changes: 3 additions & 2 deletions ArjunaJTA/jta/tests/classes/com/hp/mwtests/ts/jta/recovery/RecoveryScanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import com.arjuna.ats.internal.jta.recovery.arjunacore.XARecoveryModule;
import com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple;
import com.arjuna.ats.jta.common.jtaPropertyManager;
import com.arjuna.ats.jta.logging.RecoveryRequired;
import com.arjuna.ats.jta.logging.jtaLogger;
import org.junit.AfterClass;
import org.junit.BeforeClass;
Expand Down Expand Up @@ -91,7 +92,7 @@ public void testRecoveryMonitorWithFailure() throws Exception {

manager.scan();

assertTrue(jtaLogger.isRecoveryProblems());
assertTrue(RecoveryRequired.isRecoveryProblems());
}

@Test
Expand Down Expand Up @@ -156,7 +157,7 @@ public void start(Xid xid, int i) throws XAException {

manager.scan();

assertFalse(jtaLogger.isRecoveryProblems());
assertFalse(RecoveryRequired.isRecoveryProblems());
}

@Test
Expand Down