Skip to content

Conversation

@SivaAccionLabs
Copy link
Contributor

Dear java-grok maintainers,

Please accept this PR.

Analysis
Currently, capture is returning the unordered Map.

Resolution
Return the map with the same order as log format(pattern).

Test
Tested with different types of logs.

@SivaAccionLabs
Copy link
Contributor Author

@anthonycorbacho @paulwellnerbou @ottobackwards Please review and merge this PR.

@ottobackwards
Copy link
Contributor

First, thanks for the contribution.
I have a couple of questions, not just for you but for the others as well:

  1. Should this be optional? Have a setOrderedCaptures(boolean), then have a function
private Map<String,Object> createCaptureMap() {
       if (orderCaptures) {
           return new LinkedHashMap<>();
       }
       return new HashMap<>();
}
  1. I'm not sure that the documentation should be changed to LinkedHashMap

  2. I would expect unit tests for this or any change to be included.

@SivaAccionLabs
Copy link
Contributor Author

SivaAccionLabs commented Sep 26, 2018

@ottobackwards Thank you so much for the review. This is the most urgent and important fix for our project.

  1. Need others views too.
  2. Agree with you, it's not mandatory to change to LinkedHashMap. Will remove the changes in the documentation.
  3. Sure, will add the unit tests.

@SivaAccionLabs
Copy link
Contributor Author

SivaAccionLabs commented Sep 27, 2018

@ottobackwards Have removed the changes in the documentation and made changes in the unit tests.
Please review. Thanks.

@ottobackwards
Copy link
Contributor

Thanks @SivaMaplelabs, could you change the tests or add new tests the specifically test that the map iteration matches the capture order? That is the point.

@paulwellnerbou
Copy link
Contributor

@SivaMaplelabs , looks great. Unfortunately I don't have rights to publish it, I would have to create an own version again, as I did with https://github.com/paulwellnerbou/java-grok/releases/tag/0.1.8 and https://bintray.com/paulwellnerbou/maven/java-grok/0.1.8

@SivaAccionLabs
Copy link
Contributor Author

@anthonycorbacho Could you please review and merge this PR?

@anthonycorbacho
Copy link
Member

anthonycorbacho commented Oct 3, 2018 via email

@SivaAccionLabs
Copy link
Contributor Author

Sure. Thanks.

@anthonycorbacho anthonycorbacho merged commit 5198ad7 into thekrakken:master Oct 3, 2018
@anthonycorbacho
Copy link
Member

Reviewed and merged, thanks for your contribution 💃

@SivaAccionLabs
Copy link
Contributor Author

@anthonycorbacho Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants