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.

Sign up for GitHub

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

Jump to bottom

Reduce log level when loading blacklist to INFO from ERROR #1522

Merged
merged 4 commits into from
Jul 4, 2018
Merged

Reduce log level when loading blacklist to INFO from ERROR #1522

merged 4 commits into from
Jul 4, 2018

Conversation

dguy
Copy link
Contributor

@dguy dguy commented Jul 3, 2018 •
edited
Loading

  • Reduce log level to WARN when can't load the blacklist file.
  • Update blacklisting such that no classes are blacklisted if the file is not found or can't be loaded.
  • Change the default file such that it allows everything.

Fixes #1515

Testing done

Updated unit test

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@dguy dguy requested review from miguno, apurvam and hjafarpour July 3, 2018 11:14
@dguy
Copy link
Contributor Author

dguy commented Jul 3, 2018

@miguno i've updated this as per discussion on #1427
i.e., it the blacklist file doesn't exist then it accepts everything. Also changed the default blacklist file so it will accept everything, but has commented out examples of how to add exclusions.
Will also update the doc PR (assuming that this is merged).

apurvam
apurvam approved these changes Jul 3, 2018
View reviewed changes
Copy link
Contributor

@apurvam apurvam left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hjafarpour
Copy link
Contributor

Is there any specific reason to change it to INFO? Why not WARN?

@dguy
Copy link
Contributor Author

dguy commented Jul 4, 2018

@hjafarpour yeah you are correct, it should be warn.

@dguy dguy mentioned this pull request Jul 4, 2018
Merged
big-andy-coates
big-andy-coates approved these changes Jul 4, 2018
View reviewed changes
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

LGTM, though I think your log level is wrong...

ksql-engine/src/main/java/io/confluent/ksql/function/Blacklist.java
logger.error("failed to load resource blacklist from " + inputFile
+ " all classes will be blacklisted");
} catch (final IOException e) {
logger.warn("Failed to load resource blacklist from {}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This exception occurs if ext/resource-blacklist.txt exists, but errors occur processing it. That's an error in my book, not a warning.

I think you're right to log at INFO level if the blacklist file doesn't exist. As I would expect to be able to remove the blacklist by removing the file.

However, if I create a file with the wrong permissions for example, then it should be an ERROR.

@dguy dguy merged commit 82e7262 into confluentinc:5.0.x Jul 4, 2018
@dguy dguy deleted the resource-blacklist branch July 4, 2018 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apurvam apurvam apurvam approved these changes

@big-andy-coates big-andy-coates big-andy-coates approved these changes

@miguno miguno Awaiting requested review from miguno

@hjafarpour hjafarpour Awaiting requested review from hjafarpour

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dguy @hjafarpour @apurvam @big-andy-coates