-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: add flag to disable pull queries (MINOR) #3778
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit. LGTM
Errors.badStatement( | ||
"Pull queries are disabled on this KSQL server - please set " | ||
+ KsqlConfig.KSQL_PULL_QUERIES_ENABLE_CONFIG + "=true to enable this feature. " | ||
+ "If you intended to issue a push query, resubmit the query with EMIT CHANGES.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reword to "resubmit with EMIT CHANGES syntax" or "resubmit with EMIT CHANGES clause" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to EMIT CHANGES clause
Errors.badStatement( | ||
"Pull queries are disabled on this KSQL server - please set " | ||
+ KsqlConfig.KSQL_PULL_QUERIES_ENABLE_CONFIG + "=true to enable this feature. " | ||
+ "If you intended to issue a push query, resubmit the query with the " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just a matter of resubmitting the query with EMIT CHANGES
, right? I thought push queries went to the /query
endpoint, whereas pull queries went to the /ksql
endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and no - the CLI will handle that for you. if you submit the query to the wrong endpoint, it will also complain and tell you which endpoint you should be hitting. since all this is changing with #3742, I'd rather not bake too much of it into the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. As long as our error messages if pull/push queries are submitted to the wrong endpoint are good enough for non-CLI users to know how to respond, that works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @agavra ! LGTM.
Errors.badStatement( | ||
"Pull queries are disabled on this KSQL server - please set " | ||
+ KsqlConfig.KSQL_PULL_QUERIES_ENABLE_CONFIG + "=true to enable this feature. " | ||
+ "If you intended to issue a push query, resubmit the query with the " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. As long as our error messages if pull/push queries are submitted to the wrong endpoint are good enough for non-CLI users to know how to respond, that works for me.
import org.eclipse.jetty.http.HttpStatus.Code; | ||
import org.junit.Rule; | ||
import org.junit.Test; | ||
import org.junit.experimental.runners.Enclosed; | ||
import org.junit.rules.ExpectedException; | ||
import org.junit.runner.RunWith; | ||
import org.mockito.junit.MockitoJUnitRunner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unused Mockito imports.
Can we also get this change on |
Description
Adds a flag to make sure that allows servers to disable ad-hoc pull queries.
Testing done
Reviewer checklist