feat(bigtable): add CBT_ENABLE_DIRECTPATH=false in case user want to disable directpath#14160
feat(bigtable): add CBT_ENABLE_DIRECTPATH=false in case user want to disable directpath#14160sushanb merged 10 commits intogoogleapis:mainfrom
Conversation
sushanb
commented
Mar 11, 2026
sushanb
commented
- Enable sending the same feature flags
…disable directpath
product-auto-label
Bot
added
the
api: bigtable
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the DirectPath enablement logic, making it on-by-default and allowing it to be disabled via the CBT_ENABLE_DIRECTPATH environment variable. My review focuses on ensuring consistent behavior and improving code clarity. I've identified an inconsistency in how DirectPath is enabled across different connection pool configurations and suggested a fix. I also have some recommendations for improving comments and logging for better maintainability.
nimf
left a comment
nimf
left a comment
There was a problem hiding this comment.
Sorry, I'm confused. Why if we use bigtable channel pool we enable direct access by default and check if CBT_ENABLE_DIRECTPATH is false, while when we don't use bigtable channel pool we enable direct access only when CBT_ENABLE_DIRECTPATH is true?
| var isDirectAccess bool | ||
| if firstConn, isDirectAccess = pool.checkIfDirectAccessCompatible(); isDirectAccess { | ||
| directAccessConn, isDirectAccess := pool.checkIfDirectAccessCompatible() | ||
| disableDirectAccess := os.Getenv("CBT_ENABLE_DIRECTPATH") == "false" |
There was a problem hiding this comment.
If user disabled directpath we don't need to check if direct acccess compatible. I.e., check if directpath disabled first, if it is disabled, then directAccessConn is null and isDirectAccess is false.
There was a problem hiding this comment.
Even if user disabled, we still want to check and export the metrics.
There was a problem hiding this comment.
Let's do it in a separate goroutine then, not to delay channel pool creation.
There was a problem hiding this comment.
i think your idea is great. skipped the check if If user disabled directpath
| btransport.WithInstanceName(fullInstanceName), | ||
| btransport.WithAppProfile(config.AppProfile), | ||
| btransport.WithFeatureFlagsMetadata(ffMD), | ||
| btransport.WithFeatureFlagsMetadata(directAccessMD), |
There was a problem hiding this comment.
why do we set both featureFlagsMetadata and directAccessFeatureFlagsMetadata to directAccessMD? We don't know if we will use directpath or not yet, because we check CBT_ENABLE_DIRECTPATH in the bigtable channel pool later.
There was a problem hiding this comment.
i am making featureflags agnostic of transport. we will send the same feature flag now for both directpath and cloudpath.
There was a problem hiding this comment.
hmmm... But this means we will send DirectAccessRequested as true even if the user disables directpath?
There was a problem hiding this comment.
Yeah. I kept as it for now. I agree that it is not nice to send the featureflags if user explicitly disables direct access. I will do as a followup.
We are getting rid of legacy channel pool and will use the bigtable channel pool. that is the reason. |
| directAccessDialerOptions := make([]option.ClientOption, len(o)) | ||
| copy(directAccessDialerOptions, o) | ||
| directAccessDialerOptions = append(directAccessDialerOptions, directPathOptions...) | ||
| // enable hard bound tokens by default |
There was a problem hiding this comment.
why we don't need to check for CBT_DISABLE_DIRECTPATH_BOUND_TOKEN for BigtableConnPool but need to check it otherwise on line 242 below?
There was a problem hiding this comment.
good question. @rockspore can we now enable the direct bound token by default?
2 participants
