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.

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

[WFCORE-839] : Don't store state in InMemoryAuditLogHandlerResourceDefinition. #924

Merged
merged 1 commit into from Aug 18, 2015

Conversation

ehsavoie
Copy link
Contributor

Updating ManagedAuditLogger to be able to access the last log entries with only a real implemntation in the InMemoryAuditLogHander.

Jira: https://issues.jboss.org/browse/WFCORE-839

@wildfly-ci
Copy link

Core - Full Integration Build 1418 is now running using a merge of cf6913c

@wildfly-ci
Copy link

Windows Build 1801 is now running using a merge of cf6913c

@wildfly-ci
Copy link

Linux Build 2281 is now running using a merge of cf6913c

@wildfly-ci
Copy link

Windows Build 1801 outcome was SUCCESS using a merge of cf6913c
Summary: Tests passed: 3421, ignored: 64 Build time: 0:35:09

@wildfly-ci
Copy link

Linux Build 2281 outcome was SUCCESS using a merge of cf6913c
Summary: Tests passed: 3421, ignored: 64 Build time: 0:36:28

@wildfly-ci
Copy link

Core - Full Integration Build 1418 outcome was SUCCESS using a merge of cf6913c
Summary: Tests passed: 2983, ignored: 358 Build time: 0:48:15

@@ -58,36 +58,34 @@
}
private static final String IN_MEMORY_FORMATTER_NAME = "in-memory-formatter";
private final List<ModelNode> items;
private int maxHistory;
private volatile int maxHistory;
Copy link
Member

Choose a reason for hiding this comment

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

The rest of this class isn't thread safe. I don't think volatile is needed here.

@wildfly-ci
Copy link

Core - Full Integration Build 1450 is now running using a merge of a892dd1

@wildfly-ci
Copy link

Windows Build 1831 is now running using a merge of a892dd1

@wildfly-ci
Copy link

Linux Build 2314 is now running using a merge of a892dd1

@wildfly-ci
Copy link

Windows Build 1831 outcome was SUCCESS using a merge of a892dd1
Summary: Tests passed: 3423, ignored: 64 Build time: 0:35:11

@wildfly-ci
Copy link

Linux Build 2314 outcome was SUCCESS using a merge of a892dd1
Summary: Tests passed: 3423, ignored: 64 Build time: 0:36:14

@wildfly-ci
Copy link

Core - Full Integration Build 1450 outcome was SUCCESS using a merge of a892dd1
Summary: Tests passed: 3098, ignored: 368 Build time: 0:50:15

@bstansberry
Copy link
Contributor

We're all these changes necessary for WFCORE-839, or was there some other problem that had to be fixed? This is a lot bigger than I expected; I thought it would be trivial.

The handler doesn't seem thread safe now.

@ehsavoie
Copy link
Contributor Author

ehsavoie commented Aug 6, 2015

The handler isn't thread safe as all handlers. Thread safety is managed by ManagedAuditLoggerImpl. Since the map was already existing i switched back to using ManagedAuditLogger for all operations on the InMemoryHandler as was 'proper'.

@bstansberry
Copy link
Contributor

Thanks; I'll have another look now that I get the idea. :)

super.setMaxFailureCount(maxHistory);
this.maxHistory = maxHistory;
while (maxHistory < items.size()) {
items.remove(0);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at ManagedAuditLogger, it already has handler-specific modification methods, e.g. updateSyslogHandlerAppName, updateSyslogHandlerFacility, updateSyslogHandlerReconnectTimeout. That's a bit yuck, but since they are already there I don't see why another one can't be added for setting the max history.

ManagedAuditLogger isn't usable outside the kernel so we can be a bit casual about its API.

@wildfly-ci
Copy link

Linux Build 2398 is now running using a merge of e730e78

@wildfly-ci
Copy link

Linux Build 2398 outcome was FAILURE using a merge of e730e78
Summary: Tests failed: 1 (1 new), passed: 3400, ignored: 64 Build time: 00:37:13

Failed tests

org.jboss.as.test.integration.mgmt.access.InMemoryAuditReportTestCase.testChangeMaxHistory: java.lang.AssertionError: 
Expected: is <2>
     but: was <3>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at org.junit.Assert.assertThat(Assert.java:832)
    at org.jboss.as.test.integration.mgmt.access.InMemoryAuditReportTestCase.testChangeMaxHistory(InMemoryAuditReportTestCase.java:176)

@wildfly-ci
Copy link

WildFly pull requests Build 1827 outcome was FAILURE using a merge of e730e78
Summary: Snapshot dependencies failed: 2 (new) Build time: 00:00:01

…finition.

Updating ManagedAuditLogger to be able to access the last log entries with only a real implemntation in the InMemoryAuditLogHander.
@wildfly-ci
Copy link

Linux Build 2399 is now running using a merge of fec40cb

@wildfly-ci
Copy link

Linux Build 2399 outcome was SUCCESS using a merge of fec40cb
Summary: Tests passed: 3401, ignored: 64 Build time: 00:37:35

@wildfly-ci
Copy link

WildFly pull requests Build 1828 outcome was FAILURE using a merge of fec40cb
Summary: Snapshot dependency failed: ... WildFly Core Full - Integration (new) Build time: 00:00:01

@bstansberry bstansberry added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Aug 17, 2015
bstansberry added a commit that referenced this pull request Aug 18, 2015
[WFCORE-839] : Don't store state in InMemoryAuditLogHandlerResourceDefinition.
@bstansberry bstansberry merged commit 5f278e6 into wildfly:master Aug 18, 2015
@ehsavoie ehsavoie deleted the WFCORE-839 branch September 1, 2016 09:20
spyrkob pushed a commit to spyrkob/wildfly-core that referenced this pull request Sep 17, 2020
[JBEAP-20004] DeploymentUnitProcessor refactoring to allow sharing of ServiceName for SecurityDomain.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
4 participants