You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/11/04 15:37:55 UTC
[GitHub] [cassandra] adelapena opened a new pull request, #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
adelapena opened a new pull request, #1972:
URL: https://github.com/apache/cassandra/pull/1972
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
adelapena commented on PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#issuecomment-1305432126
> @adelapena am I understanding this correctly that a user can't specify the `reason` to be used for a specific guardrail in the config? So you can hard code it but the user can't change it at his convenience?
As it's mentioned on the ticket discussion, I don't think we should start using custom exception messages. There is no reason for that if the standard ones are clear enough, and custom messages can make it difficult to track where the exceptions come from.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
bereng commented on PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#issuecomment-1306707134
LGTM +1
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a diff in pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
bereng commented on code in PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#discussion_r1015233201
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailsTest.java:
##########
@@ -38,8 +38,8 @@
public class GuardrailsTest extends GuardrailTester
{
public static final int DISABLED = -1;
-
-
+ public static final String REASON = "Testing";
+
Review Comment:
Extra spaces
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on a diff in pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
bereng commented on code in PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#discussion_r1015181126
##########
src/java/org/apache/cassandra/db/guardrails/Predicates.java:
##########
@@ -56,16 +56,18 @@
* Creates a new {@link Predicates} guardrail.
*
* @param name the identifying name of the guardrail
+ * @param reason the optional description of the reason for guarding the operation
Review Comment:
formatting
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] josh-mckenzie commented on pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
josh-mckenzie commented on PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#issuecomment-1305593967
Agree. My understanding was that the "user custom decoration" approach was a no-go.
My understanding of the consensus:
1. Make our default error messages (much, _**much**_ better)
2. Add a config option, probably in the .yaml, for "guardrail_contact" (or some other better name) where you can provide an email address to hit up for questions about operator configs on clusters
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
bereng commented on PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#issuecomment-1305333654
@adelapena am I understanding this correctly that a user can't specify the `reason` to be used for a specific guardrail in the config? So you can hard code it but the user can't change it at his convenience?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] josh-mckenzie commented on a diff in pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
josh-mckenzie commented on code in PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#discussion_r1019504964
##########
src/java/org/apache/cassandra/db/guardrails/Guardrail.java:
##########
@@ -138,8 +143,19 @@ protected void fail(String message, String redactedMessage, @Nullable ClientStat
@VisibleForTesting
String decorateMessage(String message)
{
- // Add a prefix to error message so user knows what threw the warning or cause the failure
- return String.format("Guardrail %s violated: %s", name, message);
+ // Add a prefix to error message so user knows what threw the warning or cause the failure.
+ String decoratedMessage = String.format("Guardrail %s violated: %s", name, message);
+
+ // Add the reason for the guardrail triggering, if there is any.
+ if (reason != null)
+ {
+ if (message.endsWith("."))
Review Comment:
ultra-nit can do on commit if you want: ternary may make this a touch cleaner.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
bereng commented on PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#issuecomment-1305473920
Yep I read the ticket. It is one thing to go 100% for custom messages which I agree would make it difficult to track errors.
But it is a different one to allow users to add to the message. Josh's idea of adding an email i.e. Or yours later to have a sort of a global property, etc
It seems the idea of allowing the user to decorate the message somehow is a good one, accepted but not included in the PR? If I am not missing anything ofc!
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
adelapena commented on PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#issuecomment-1305518238
I'm not sure that the idea of allowing the user to decorate the message is accepted on that discussion. For sure I don't like it too much because I see it as unnecessary if the standard message is clear enough.
As for adding a generic message fragment with contact email, etc., I think that should be common to all user-facing error messages, and not only for guardrails or AF. That should IMO done in a separate ticket, since here we are dealing with the confusing AF message and the reasons to forbid it.
@josh-mckenzie wdyt?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bereng commented on pull request #1972: CASSANDRA-17967 trunk: Add reason for guardrail triggering
Posted by GitBox <gi...@apache.org>.
bereng commented on PR #1972:
URL: https://github.com/apache/cassandra/pull/1972#issuecomment-1305638853
Ok so we're doing that bit in another ticket. Let me go through this a bit more but it looks like a +1 then so far
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org