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

[incubator-kie-issues#1627] Change log level from error to debug inside ImportDMNResolverUtil #6172

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

yesamer
Copy link
Contributor

@yesamer yesamer commented Nov 26, 2024

Closes apache/incubator-kie-issues#1627

Follow up of #6150, discovered during apache/incubator-kie-issues#1627 testing.

Based on the current architecture, the resolveImportDMN(Import, Collection<T>, Function<T, QName>) is called twice, which means that an error logged in the first call is inconsistent with the second call, where the behavior works as expected.

As a consequence, all log levels should be set as DEBUG, until a definitive fix in the architecture.

@yesamer yesamer added the DMN label Nov 26, 2024
@yesamer yesamer changed the title [NO_ISSUE] Change log level from error to debug inside ImportDMNResolverUtil [incubator-kie-issues#1627] Change log level from error to debug inside ImportDMNResolverUtil Nov 26, 2024
Copy link
Contributor

@gitgabrio gitgabrio left a comment

Choose a reason for hiding this comment

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

Hi @yesamer
on second thought, I think we have to change the overall approach here.
That method get's invoked twice, once with an empty Collection<T> dmns and once with a populated one.
The "misleading" logs are related to the first one.
So, I think it is better that, when an empty dmns collections is recieved, the method immediately returns back with an Either.of left, and debug messaging explaining it.
If, instead, a populated collection is received, all failures on finding imported model should be logged as error: wdyt ?

@yesamer
Copy link
Contributor Author

yesamer commented Nov 26, 2024

@gitgabrio The improvement you suggested makes sense, but I would manage it in this ticket apache/incubator-kie-issues#1383 (please feel free to update it) and provide a complete refactoring of the feature

@gitgabrio
Copy link
Contributor

gitgabrio commented Nov 26, 2024

@yesamer
The ticket you mention has a more broader impact and it is not sure when it could be done.
On the other side, reviewing this PR, I realized that we are dealing two different uses cases (empty or populated dmns) with the same logic.
In one case, the "error" level is required, but now (even with my previous one) we are logging everything as debug, which is wrong, IMO.
Since it is just a matter of a couple of lines in the very same method already touched by this PR, I think it would be better to fix that with this PR (and reverting the "debug" messages to "error").

@yesamer
Copy link
Contributor Author

yesamer commented Nov 26, 2024

@gitgabrio Done, thank you

Copy link
Contributor

@gitgabrio gitgabrio left a comment

Choose a reason for hiding this comment

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

Thanks @yesamer
lgtm!

@yesamer
Copy link
Contributor Author

yesamer commented Nov 27, 2024

@baldimir Can you please review it?

@yesamer yesamer merged commit c0c300c into apache:main Nov 28, 2024
10 checks passed
@yesamer yesamer deleted the NO_ISSUE_log_lvevl_import_dmn branch November 28, 2024 13:16
rgdoliveira pushed a commit to rgdoliveira/drools that referenced this pull request Dec 2, 2024
…de ImportDMNResolverUtil (apache#6172)

* change log level

* change log level

* change log level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot include models: I get a "failed to import" when running a test with imported models
3 participants