[GitHub] lucene-solr issue #342: SOLR-12120: New AuditLoggerPlugin type allowing cust...
Github user tflobbe commented on the issue: https://github.com/apache/lucene-solr/pull/342 LGTM, thanks for addressing my comments --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #342: SOLR-12120: New AuditLoggerPlugin type allowing cust...
Github user janhoy commented on the issue: https://github.com/apache/lucene-solr/pull/342 Pushed another commit simplifying the if checks into `auditIfConfigured()`. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #342: SOLR-12120: New AuditLoggerPlugin type allowing cust...
Github user janhoy commented on the issue: https://github.com/apache/lucene-solr/pull/342 Pushed changes to the PR, addressing most of the comments. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #342: SOLR-12120: New AuditLoggerPlugin type allowing cust...
Github user tflobbe commented on the issue: https://github.com/apache/lucene-solr/pull/342 > Perhaps this is better? I wasn't really asking for any change, I don't like it much, but I know that's how the related functionality is working. One option would be to create a method like `logAudit(EventType,Request, Context...)` and do the null check inside? IDK, just a thought. > so far they are mainly unit tests for the raw java code I like how simple they are. My question was in relation to things like `reproduce with ant test -Dtests.seed=1234`, in addition to the thread leak checking and things like that included when extending LuceneTestCase. This was really more a question than a suggestion for change from my side. > perhaps using Mockito See SOLR-11606 before using Mockito > Do you think it makes sense to randomise audit logging on one of the larger tests such as SolrCloudTestCase? Your call. I think the functionality is straight forward enough that simpler tests can give you enough coverage... --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #342: SOLR-12120: New AuditLoggerPlugin type allowing cust...
Github user janhoy commented on the issue:
https://github.com/apache/lucene-solr/pull/342
> I wish we wouldn't have to do all those if (cores.getAuditLoggerPlugin()
!= null)
Perhaps this is better?
if (auditEnabled()) { ... }
> are you intentionally not extending any of the base test classes in your
tests
Well, yes, the tests need more work, so far they are mainly unit tests for
the raw java code. Need to add more integration tests that run real request
traffic and somehow validate that logging takes place. Guess that can be done
with a `MockAuditLogger` which can then be queried at the end of the test, or
perhaps using Mockito ?
Do you think it makes sense to randomise audit logging on one of the larger
tests such as `SolrCloudTestCase`?
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
