-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Simplify TestingConnectorBehavior declarations #16825
Simplify TestingConnectorBehavior declarations #16825
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.
I agree. Makes sense. Will revisit once CI results are available.
d27c8d9
to
1898934
Compare
5a6f2ca
to
8569550
Compare
@@ -15,8 +15,8 @@ | |||
|
|||
import static io.trino.plugin.pinot.TestingPinotCluster.PINOT_LATEST_IMAGE_NAME; | |||
|
|||
public class TestPinotWithoutAuthenticationIntegrationLatestVersionNoGrpcConnectorSmokeTest |
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.
After i made this class names a little shorter i realized we don't have non-Smoke BCT coverage for Pinot yet.
@elonazoulay is there an issue for this already?
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.
There is no issue but I can create a BCT for pinot.
8569550
to
36cc6d0
Compare
CI #17033 |
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.
Left minor comments. Please ignore if those codes are intentional.
@@ -64,11 +64,6 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | |||
case SUPPORTS_CREATE_MATERIALIZED_VIEW: | |||
return true; |
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.
Can we remove SUPPORTS_CREATE_VIEW
& SUPPORTS_CREATE_MATERIALIZED_VIEW
?
case SUPPORTS_DELETE: | ||
return true; |
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.
Can we remove SUPPORTS_DELETE
?
c32e02f
to
8ff3371
Compare
b5c8cd3
to
900f21e
Compare
The "Integration" looked like a remnant of the old naming convention before BCT and BCST were introduced. The "LatestVersion" is usually written as just "latest". Remove other unnecessary words and make the class names readable.
Use `true` or a related behavior as the default rather than `false`. Few cases being off by default remain, but generally true as the default is much easier to reason about. False defaults were usually a result of incremental additions, where it's easier to add a behavior that is not widely adopted, but are harder to think about in a long run.
900f21e
to
c93bd71
Compare
CI #13995 |
Use
true
or a related behavior as the default rather thanfalse
. Few cases being off by default remain, but generally true as the default is much easier to reason about. False defaults were usually a result of incremental additions, where it's easier to add a behavior that is not widely adopted, but are harder to think about in a long run.cc @hashhar @ebyhr