Re: [PR] MINOR: Fix uses of ConfigException [kafka]
github-actions[bot] commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1933309585 This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
jolshan commented on code in PR #14721:
URL: https://github.com/apache/kafka/pull/14721#discussion_r1388376812
##
core/src/main/scala/kafka/utils/PasswordEncoder.scala:
##
@@ -130,7 +131,7 @@ class EncryptingPasswordEncoder(
val decrypted = cipher.doFinal(encryptedPassword)
new String(decrypted, StandardCharsets.UTF_8)
} catch {
- case e: Exception => throw new ConfigException("Password could not be
decoded", e)
+ case e: Exception => throw new InvalidConfigurationException("Password
could not be decoded", e)
Review Comment:
took a quick look at usages -- looks like dynamic broker configs where it is
caught and handled and the ZK migration code that doesn't seeme to specifically
handle either error case. I feel ok with this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
jolshan commented on code in PR #14721:
URL: https://github.com/apache/kafka/pull/14721#discussion_r1388381937
##
clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java:
##
@@ -99,7 +100,7 @@ public void configure(Map configs) throws
KafkaException {
try {
SslEngineValidator.validate(builder, builder);
} catch (Exception e) {
-throw new ConfigException("A client SSLEngine created with the
provided settings " +
+throw new InvalidConfigurationException("A client SSLEngine
created with the provided settings " +
Review Comment:
Usages here are a bit tricky to track. So many overrided "configure" methods
that go in circles
@bob-barrett do you have a simplified flow for this to confirm it won't
cause compatibility issues?
##
clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java:
##
@@ -99,7 +100,7 @@ public void configure(Map configs) throws
KafkaException {
try {
SslEngineValidator.validate(builder, builder);
} catch (Exception e) {
-throw new ConfigException("A client SSLEngine created with the
provided settings " +
+throw new InvalidConfigurationException("A client SSLEngine
created with the provided settings " +
Review Comment:
Usages here are a bit tricky to track. So many overrided "configure" methods
that go in circles
@bob-barrett do you have a simplified flow for this to confirm it won't
cause compatibility issues?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
jolshan commented on code in PR #14721:
URL: https://github.com/apache/kafka/pull/14721#discussion_r1388376812
##
core/src/main/scala/kafka/utils/PasswordEncoder.scala:
##
@@ -130,7 +131,7 @@ class EncryptingPasswordEncoder(
val decrypted = cipher.doFinal(encryptedPassword)
new String(decrypted, StandardCharsets.UTF_8)
} catch {
- case e: Exception => throw new ConfigException("Password could not be
decoded", e)
+ case e: Exception => throw new InvalidConfigurationException("Password
could not be decoded", e)
Review Comment:
took a quick look at usages -- looks like dynamic broker configs where it is
caught and handled and the ZK migration code that doesn't seeme to specifically
handle either error case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
C0urante commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1804121753 I realize that. I think preserving the existing exception type is more valuable (and correct) here. We'd also be dropping the stack trace for the top-level exception (see [ApiException::fillInStackTrace](https://github.com/apache/kafka/blob/39c6170aa96e4c9840ac469d1b43bb059f0513af/clients/src/main/java/org/apache/kafka/common/errors/ApiException.java#L45-L49)) if we switched to using `InvalidConfigurationException`. If we really want to preserve all information (including stack traces for the exception and its cause), we should use an exception type that doesn't subclass `ApiException`. IMO it's not worth it to switch to a third thing, though, and it's better to preserve the top-level exception's stack trace than the cause's stack trace. If absolutely necessary, we can manually construct the caused-by string in the message we pass to `ConfigException`, in addition to the message of the cause (and possibly the type, now that I think about it). But I don't think that's necessary here and it strikes me as overkill. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
bob-barrett commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1804101882 I dropped the changes in the Connect code, because I wasn't confident in what the implications could be. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
bob-barrett commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1804100758 @C0urante we would keep the message in that case but not the nested exception, so we would lose the stack trace. We actually already keep the message, because `ConfigException` tries to stick it into the message string as the config value, and that implicitly calls `toString()` on the exception, which returns the message (at least in the case of this that I've seen, obviously it depends on the causing exception's `toString` implementation). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
C0urante commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1804080696 IMO it'd be cleaner if we could keep the same exception type. We could concatenate the existing messages with the cause's message (if non-null); for example: ```java String message = "A client SSLEngine created with the provided settings " + "can't connect to a server SSLEngine created with those settings." if (e.getMessage() != null) message += ": " + e.getMessage(); throw new ConfigException(message); ``` A follow-up item (that requires a KIP but hopefully shouldn't be too controversial) could be to add a new `ConfigException(String message, Throwable cause)` constructor, which would allow us to preserve the complete stack trace of the cause instead of just the message. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
jolshan commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1803010902 > None of the other changes can wind up in an API response. So for compatibility, we should drop the DymamicBrokerConfig change, but the others should be safe to keep. If we want to be extra safe about it, the only one I actually care about is SslFactory, I'm happy to drop the other changes. I'd be happy with just removing the DynamicBrokerConfig change if we feel good about the others not causing issues. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
bob-barrett commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1802992442 > Looks reasonable. Just want to confirm that all of these are thrown and not returned by any response apis? Good point, the `DynamicBrokerConfig` change can wind up bubbling up to https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/ConfigAdminManager.scala#L199 and getting returned as `INVALID_CONFIG` instead of `INVALID_REQUEST`, which the comment specifically calls out as something we shouldn't do. `SslFactory` can throw either on broker startup or in response to a dynamic config update, in which case it will actually wind up getting caught and re-thrown by `DynamicBrokerConfig`. None of the other changes can wind up in an API response. So for compatibility, we should drop the `DymamicBrokerConfig` change, but the others should be safe to keep. If we want to be extra safe about it, the only one I actually care about is `SslFactory`, I'm happy to drop the other changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
jolshan commented on PR #14721: URL: https://github.com/apache/kafka/pull/14721#issuecomment-1802978156 Looks reasonable. Just want to confirm that all of these are thrown and not returned by any response apis? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
bob-barrett commented on code in PR #14721:
URL: https://github.com/apache/kafka/pull/14721#discussion_r1387343761
##
core/src/main/scala/kafka/utils/PasswordEncoder.scala:
##
@@ -20,12 +20,12 @@ import java.nio.charset.StandardCharsets
import java.security.{AlgorithmParameters, NoSuchAlgorithmException,
SecureRandom}
import java.security.spec.AlgorithmParameterSpec
import java.util.Base64
-
Review Comment:
Almost certainly done by my IDE, I'll fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Fix uses of ConfigException [kafka]
jolshan commented on code in PR #14721:
URL: https://github.com/apache/kafka/pull/14721#discussion_r1387343457
##
core/src/main/scala/kafka/utils/PasswordEncoder.scala:
##
@@ -20,12 +20,12 @@ import java.nio.charset.StandardCharsets
import java.security.{AlgorithmParameters, NoSuchAlgorithmException,
SecureRandom}
import java.security.spec.AlgorithmParameterSpec
import java.util.Base64
-
Review Comment:
small nit: did we mean to remove this new line?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] MINOR: Fix uses of ConfigException [kafka]
bob-barrett opened a new pull request, #14721: URL: https://github.com/apache/kafka/pull/14721 There are some places in the code where we create `ConfigException` object by passing in a message String and an Exception. This is a standard pattern for exceptions, but it is incorrect for `ConfigException`, which interprets these arguments as a config key and config value. When we do this, the original exception is lost, since it is not interpreted as a `Throwable`, which can make debugging configuration errors difficult. This PR replaces such uses of `ConfigException` with `InvalidConfigurationException`. ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
