Skip to content

PropertiesModule: Print properties, processors, totalMemory on startup.#2452

Merged
himanshug merged 1 commit intoapache:masterfrom
gianm:print-properties
Feb 11, 2016
Merged

PropertiesModule: Print properties, processors, totalMemory on startup.#2452
himanshug merged 1 commit intoapache:masterfrom
gianm:print-properties

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Feb 11, 2016

Should help with debugging issues from the mailing lists.

@fjy fjy added this to the 0.9.0 milestone Feb 11, 2016
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 11, 2016

👍

himanshug added a commit that referenced this pull request Feb 11, 2016
PropertiesModule: Print properties, processors, totalMemory on startup.
@himanshug himanshug merged commit 47d48e1 into apache:master Feb 11, 2016
@gianm gianm deleted the print-properties branch February 11, 2016 23:01
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 12, 2016

@gianm can we backport this?

);

for (String propertyName : Ordering.natural().sortedCopy(props.stringPropertyNames())) {
log.info("* %s: %s", propertyName, props.getProperty(propertyName));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will print aws credentials in log, please remove.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same with db user pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO printing out the property names should be fine, but we should not print out property values unless we have a way to differentiate secure from insecure properties.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you don't have to put your properties there, there are ways to read these credentials from file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then we should remove support for the way that puts plain text credentials into the log by default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

druid does not provide any default way to supply the db credentials other than passing them in properties file.

@drcrallen
Copy link
Copy Markdown
Contributor

@gianm acceptable alternatives would make this configurable as opt-in, or simply printing out the keys but not the values.

@nishantmonu51
Copy link
Copy Markdown
Member

+1, on making it configurable.

@drcrallen
Copy link
Copy Markdown
Contributor

@gianm I'd be ok with making the setting to enable it have it set equal to TRUE in the template runtime.properties that get packaged by default, but in the absence of a setting printing of the value of properties should be disabled.

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Feb 12, 2016

@drcrallen sounds good, will do another PR.

gianm added a commit to gianm/druid that referenced this pull request Feb 12, 2016
Off by default, but enabled in the example config files. See also apache#2452.
drcrallen pushed a commit to metamx/druid that referenced this pull request Feb 18, 2016
Off by default, but enabled in the example config files. See also apache#2452.
navis pushed a commit to navis/druid that referenced this pull request Jun 10, 2016
Off by default, but enabled in the example config files. See also apache#2452.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants