Adding table config overrides for disabling groovy #8196
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timsants
force-pushed
the
groovy_table_override
branch
3 times, most recently
from
22f9f34
to
17880f6
Compare
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
Outdated
Show resolved
Hide resolved
timsants
force-pushed
the
groovy_table_override
branch
from
17880f6
to
8d43486
Compare
Codecov Report
@@ Coverage Diff @@
## master #8196 +/- ##
============================================
- Coverage 70.97% 69.77% -1.20%
- Complexity 4313 4314 +1
============================================
Files 1626 1626
Lines 84851 84943 +92
Branches 12790 12787 -3
============================================
- Hits 60221 59273 -948
- Misses 20496 21542 +1046
+ Partials 4134 4128 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
timsants
force-pushed
the
groovy_table_override
branch
from
8d43486
to
e888e84
Compare
timsants
force-pushed
the
groovy_table_override
branch
from
e888e84
to
9774962
Compare
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QueryConfig.java
Outdated
Show resolved
Hide resolved
| tableConfig.setQueryConfig(new QueryConfig(5L, Boolean.TRUE)); | ||
| updateTableConfig(tableConfig); | ||
|
|
||
| reloadOfflineTable(getTableName()); |
There was a problem hiding this comment.
We don't need to reload the table because that won't update the table config on broker, which is ZK watcher based. We can keep sending queries using TestUtils.waitForCondition() until the query fails.
There was a problem hiding this comment.
I added the waitForCondition but for some reason the testDistinctCountHll test is flaky when I remove the reload. Any ideas why this is happening?
...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
Outdated
Show resolved
Hide resolved
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
Outdated
Show resolved
Hide resolved
timsants
force-pushed
the
groovy_table_override
branch
from
805c666
to
e9a7c35
Compare
timsants
force-pushed
the
groovy_table_override
branch
from
e9a7c35
to
dd4c2e4
Compare
snleee
reviewed
Feb 17, 2022
| @@ -37,14 +37,27 @@ | |||
| // because by the time the server times out, the broker should already timed out and returned the response. | |||
| private final Long _timeoutMs; | |||
|
|
|||
| // Table config override for disable Groovy broker property | |||
| private final Boolean _disableGroovy; | |||
There was a problem hiding this comment.
According to #7966, it looks that we would eventually want to disable Groovy by default. What's our plan on changing the default behavior?
There was a problem hiding this comment.
This PR is backward compatible. We can probably submit a separate small backward incompatible PR to simply change the default value for it
Jackie-Jiang
added
the
release-notes
Jackie-Jiang
approved these changes
Feb 17, 2022
xiangfu0
pushed a commit
to xiangfu0/pinot
that referenced
this pull request
Feb 23, 2022
Introduced new query config value `disableGroovy` which is a table config override for enabling/disable Groovy queries.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a table config override for disabling Groovy in query and ingestion. This is a follow up to #8159. This PR introduces a new boolean config value in the queryConfig named
disableGroovy. If it is not set, then it will use the disable Groovy query configs defined in the broker configs.Note that this table override does not apply to ingestion Groovy functions given that the Groovy is defined within the table config as well.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Introduced new query config value
disableGroovywhich is a table config override for enabling/disable Groovy queries.Documentation
Will update documentation after this is merged.