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

MINOR: Add method to Herder to control logging of connector configs during validation #8263

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

chia7712
Copy link
Contributor

Connector validation API is useful in validating connector configs before starting connector. And our application counts on it to generate friendly feedback to users. However, Connector validation API always log all configs when handling validation request since it creates AbstractConfig to check the overridable configs (introduced by #6624) and AbstractConfig, by default, logs all configs. In our case, the worker logs are filled with the following messages when there are a log of validation requests.

INFO AbstractConfig values:
xxx
xxx
xxx

It seems to me the response of Connector validation API is good enough. By contrast, the logs are a bit noisy and useless.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@ConcurrencyPractitioner
Copy link
Contributor

@chia7712
Copy link
Contributor Author

the flaky "KafkaBasedLogTest.testSendAndReadToEnd" is already fixed by #8255

@chia7712 chia7712 force-pushed the fix_unused_logging_in_validation branch from 314a11e to 256f1d5 Compare March 11, 2020 03:20
@chia7712
Copy link
Contributor Author

@mageshn Could you take a look? thanks!

@chia7712
Copy link
Contributor Author

the related code were introduced by #6624. @ijuma Could you take a look?

Copy link
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

I don't think I agree with the justification of this change.

If by Connect validation API you are referring to the /config/validate endpoint of the Connect REST API, I'd argue that just a response from this endpoint, as a way to list the connector configs, is hardly sufficient.

To the contrary, Worker logs that contain a connector's configuration among other valuable information are the single most important source of information when troubleshooting connectors.

I'd suggest closing without merging, unless I'm missing something here.

@chia7712
Copy link
Contributor Author

@kkonstantine thanks for suggestion!

If by Connect validation API you are referring to the /config/validate endpoint of the Connect REST API, I'd argue that just a response from this endpoint, as a way to list the connector configs, is hardly sufficient.
To the contrary, Worker logs that contain a connector's configuration among other valuable information are the single most important source of information when troubleshooting connectors.

I totally agree that the connector's configuration are the useful information when troubleshooting connectors. However, the "configuration" I mentioned is not used by any connectors. Instead, it is just a temporary result. For example, users may use /config/validate multi-times before sending creation request and worker log are full of INFO AbstractConfig values.

At any rate, could we change the log level from INFO to DEBUG at least for this case?

@kkonstantine
Copy link
Contributor

I totally agree that the connector's configuration are the useful information when troubleshooting connectors. However, the "configuration" I mentioned is not used by any connectors. Instead, it is just a temporary result. For example, users may use /config/validate multi-times before sending creation request and worker log are full of INFO AbstractConfig values.

At any rate, could we change the log level from INFO to DEBUG at least for this case?

Unfortunately AbstractHerder#validateConnectorConfig is not only used when you hit the endpoint {connector}/config/validate. It's also used by other endpoints when you create or reconfigure a connector.

Maybe you want to describe more your use case, but taking a step back, by default Connect now has the option to export the logs in files and rotate those files - a setting that can be configured even further to accommodate situations where the log becomes more verbose.

I'm not sure that by silencing this log we are gaining a lot.

@chia7712
Copy link
Contributor Author

Unfortunately AbstractHerder#validateConnectorConfig is not only used when you hit the endpoint {connector}/config/validate. It's also used by other endpoints when you create or reconfigure a connector.

My idea is to add an new construct to AbstractConfig for this case so log level is "able" to be DEBUG. for example, new AbstractConfig (xxx, xxx, debugLevel = true)

Maybe you want to describe more your use case

In short, our UI offers the visual "feedback" of configs validation. Users can get the validation response immediately after entering any config change (for example, users see the suggestion "this field must be a number" immediately after they enter a string). In order to address the function, we send a bunch of validation request to worker.. The worst case - a lot of users are configuring their connectors - there is a log file contains only INFO AbstractConfig values...

I'm not sure that by silencing this log we are gaining a lot.

I'm going to close this PR as a result of objection.

but taking a step back, by default Connect now has the option to export the logs in files and rotate those files - a setting that can be configured even further to accommodate situations where the log becomes more verbose.

@kkonstantine thanks for suggestions !

@chia7712 chia7712 closed this Mar 22, 2020
@ijuma
Copy link
Contributor

ijuma commented Mar 22, 2020

I actually agree with @chia7712 that it's pretty surprising to see these.logs when calling a validate endpoint.

@kkonstantine
Copy link
Contributor

Thanks for the follow up @ijuma.
Indeed, maybe I wasn't clear in the beginning, but I was referring to the proposed implementation in its current form, that would silence these logs under all circumstances (roughly calls to validate, create, and configure a connector).

As we were discussing the issue with @chia7712, I also didn't like the fact that validating a connector could generate this type of noise. In my last comment I highlighted the need to decouple the calls to Herder#validateConnectorConfig depending on whether this call happens from the {connector}/config/validate endpoint, or while creating or reconfiguring a connector.

Such decoupling would probably involve adding a method
ConfigInfos validateConnectorConfig(Map<String, String> connectorConfig, boolean doLog);
next to the existing one, to the Herder interface or the AbstractHerder class only.

What do you all think?

@ijuma
Copy link
Contributor

ijuma commented Mar 22, 2020

Sounds good to me.

@kkonstantine
Copy link
Contributor

@chia7712 I'm reopening this PR. Feel free to amend the commit to follow the more detailed approach described above.

@kkonstantine kkonstantine reopened this Mar 22, 2020
@chia7712 chia7712 force-pushed the fix_unused_logging_in_validation branch from 256f1d5 to 9658d6a Compare March 22, 2020 17:38
@chia7712
Copy link
Contributor Author

I'm reopening this PR. Feel free to amend the commit to follow the more detailed approach described above.

@kkonstantine Got it. the commit is updated!

Copy link
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

Thanks @chia7712 !
Left a few comments re: method overrides.

* @param doLog set it to true to log all connectorConfig with INFO level. false to log nothing.
* Noted: there are many endpoints requiring this method but not all configs are worth being logged.
*/
ConfigInfos validateConnectorConfig(Map<String, String> connectorConfig, boolean doLog);
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it's the new method that needs a default implementation if we choose to add it to this interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestions!

Just curious, is Herder.java a public interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I trace the history of Herder.java. It seems to me Herder.java is not public interface so it should be fine to add new methods directly. For example, the commit added three new methods without default implementation.

Am I missing something here?

Copy link
Contributor

Choose a reason for hiding this comment

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

First, to your first question, no this interface is not public api, but as a general principle we are trying to maintain compatibility regardless, wherever it makes sense. Here, keeping both methods makes the most sense.

Now, w/r/t the commit that you mention, indeed there's a difference.
That commit was the implementation of KIP-558. And that's how we document changes to public interfaces, by writing and discussing publicly KIPs. You may read more here: https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals

In this PR a KIP would be an overkill, and given that this is not public interface, we can easily maintain compatibility the way I described above. The other difference is that here a default implementation makes sense, while in KIP-558 it didn't make much sense.

Hope this helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kkonstantine thanks for the great explanation!!! I have addressed your comments in the latest commit. Please take a look. thanks.

@@ -78,7 +78,9 @@ public ConfigInfos validateConfigs(
);
}

return herder.validateConnectorConfig(connectorConfig);
return herder.validateConnectorConfig(connectorConfig,
// the validated configs don't need to be logger.
Copy link
Contributor

Choose a reason for hiding this comment

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

A per argument inline comment is not something I've seen being used widely in Connect's codebase.

Comment on lines 81 to 83
return herder.validateConnectorConfig(connectorConfig,
// the validated configs don't need to be logger.
false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return herder.validateConnectorConfig(connectorConfig,
// the validated configs don't need to be logger.
false);
// the validated configs don't need to be logger.
return herder.validateConnectorConfig(connectorConfig, false);

@@ -299,7 +299,7 @@ public StatusBackingStore statusBackingStore() {
}

@Override
public ConfigInfos validateConnectorConfig(Map<String, String> connectorProps) {
public ConfigInfos validateConnectorConfig(Map<String, String> connectorProps, boolean doLog) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the other comment, we'll need to keep the @Override of the previous method, and just have it call the new method that will also be overridden.

@chia7712 chia7712 force-pushed the fix_unused_logging_in_validation branch 2 times, most recently from e3e277a to d054740 Compare March 23, 2020 12:04
@chia7712
Copy link
Contributor Author

flaky StandbyTaskEOSIntegrationTest is traced by #8330

@chia7712
Copy link
Contributor Author

retest this please

1 similar comment
@kkonstantine
Copy link
Contributor

retest this please

@chia7712 chia7712 force-pushed the fix_unused_logging_in_validation branch from d054740 to dbd10d4 Compare March 24, 2020 03:43
Copy link
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

Latest build was successful.
Added a last commit with minor doc fixes.

LGTM
Merging to trunk.
Thanks @chia7712 !

@kkonstantine kkonstantine merged commit a5c2578 into apache:trunk Mar 24, 2020
@kkonstantine kkonstantine changed the title Minor: remove useless log messages from connector configs validation MINOR: Add method to Herder to control logging of connector configs during validation Mar 24, 2020
@chia7712 chia7712 deleted the fix_unused_logging_in_validation branch March 25, 2024 15:23
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.

4 participants