-
Notifications
You must be signed in to change notification settings - Fork 17
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
NTA-89: Add a keyword filter #80
base: main
Are you sure you want to change the base?
Conversation
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/75/ |
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/76/ |
inputStream = getClass().getClassLoader().getResourceAsStream(propFileName); | ||
|
||
if (inputStream != null) { | ||
prop.load(inputStream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time getPropValues() get's called you are re-reading the properties. How about just doing it once in a constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/77/ |
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/78/ |
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/79/ |
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/80/ |
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/81/ |
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/82/ |
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/83/ |
filterKeywords = new ArrayList<>(); | ||
|
||
String propFileName = "filter.properties"; | ||
logger.warn(getClass().getClassLoader().getResource("")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log message does not server any purpose.
Started testing this pull request: http://albany.eng.hst.ams2.redhat.com/job/pulls-transaction-analyser/84/ |
@Yuan-Hu The changes are looking much better now, thank you for you patience. @zhfeng The associated jira (NTA-89) does not give enough information to determine whether the implementation does what is required. Is the intention to only consider for parsing those lines that match the comma separated keywords list? Or is this some kind of processing pipeline where other filtering has already happened? |
@mmusgrov Also thanks for your continuous advice. And NTA-89 is created by myself for the former intention you mentioned. |
@@ -0,0 +1,2 @@ | |||
keywords=TransactionImple,Periodic Recovery,TransactionSynchronizationRegistryImple | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you decide on this list of keywords. If they are controlling which log lines are passed on for further processing then there are many others that need to be included, for example BasicAction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, they are controlling the lines we don't need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly the reason why the JIRA is incomplete. A good JIRA will explain the requirement and perhaps where the requirement originates from.
Why is there not a need for filters that block lines and other filters that match lines. If there is such a need then you would need at least two property names, for example: matching_keywords and blocked_keywords
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/170/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/171/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/172/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/173/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/174/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/175/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/176/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/177/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/178/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/179/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/180/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/181/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/182/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/183/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/184/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/185/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/186/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/191/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/1/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/4/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/5/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/6/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/2/ |
1 similar comment
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/2/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/3/ |
Started testing this pull request: http://narayanaci1.eng.hst.ams2.redhat.com/job/transaction-analyser-pulls/1/ |
@Yuan-Hu I am closing this PR since the Blacktie project is now archived (https://groups.google.com/g/narayana-users/c/k0EgFlS6VJc/m/6DcliSBfEAAJ). But thanks very much for your PR. What are you using for a transaction manager now? |
Sorry @Yuan-Hu I got the analyser confused with the archived BlackTie project - I have re-opened your PR. |
https://issues.jboss.org/browse/NTA-89