Skip to content

Conversation

@showuon
Copy link
Member

@showuon showuon commented Nov 8, 2024

What is the purpose of the change

In the doc, we didn't explicitly instruct users how to change log level dynamically for operator/job manager/task manager. This is important for troubleshooting, and it is already built-in config in Flink core.

Brief change log

  • In log4j-console.properties and log4j-operator.properties in Helm, added a hint to demo how to set config to change config dynamically.
  • In the doc, added a hint to demo how to set config to change config dynamically.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: no
  • Core observer or reconciler logic that is regularly executed: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@showuon showuon force-pushed the FLINK-36677 branch 2 times, most recently from 260878d to 8db1a4b Compare November 11, 2024 02:25
Comment on lines 59 to 62
Note, you should remember to escape some special characters in your `--set` lines, for example:
```
helm install --set defaultConfiguration."log4j-operator\.properties"=rootLogger.level\=DEBUG flink-kubernetes-operator helm/flink-kubernetes-operator
```
Copy link
Member Author

Choose a reason for hiding this comment

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

This took me some time to figure it out. I think it'd be great if we explicitly document it.

Copy link
Contributor

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

LGTM

@showuon
Copy link
Member Author

showuon commented Nov 15, 2024

@gyfora @mxm , please help take a look when available. Thanks.

Copy link
Contributor

@mxm mxm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! We usually just restart the operator deployment in case of logging changes, but this is cleaner.

@showuon
Copy link
Member Author

showuon commented Nov 15, 2024

@mxm , thanks for the comments. PR updated. Please take a look again. Thanks.

Copy link
Contributor

@mxm mxm left a comment

Choose a reason for hiding this comment

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

@gyfora Are you ok with disabling the monitoring by default?

@mxm mxm merged commit efa3f10 into apache:main Nov 19, 2024
98 checks passed
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.

3 participants