Copilot commented on code in PR #17405:
URL: https://github.com/apache/pinot/pull/17405#discussion_r2647084391
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java:
##########
@@ -613,7 +622,13 @@ public SegmentUploadContext(PinotTaskConfig
pinotTaskConfig,
Map<String, String> configs = pinotTaskConfig.getConfigs();
_tableNameWithType = configs.get(MinionConstants.TABLE_NAME_KEY);
_uploadURL = configs.get(MinionConstants.UPLOAD_URL_KEY);
- _authProvider =
AuthProviderUtils.makeAuthProvider(configs.get(MinionConstants.AUTH_TOKEN));
+ // Prefer the runtime minion AuthProvider; fallback to token if needed.
+ AuthProvider runtimeProvider = MINION_CONTEXT.getTaskAuthProvider();
+ if (runtimeProvider != null && !(runtimeProvider instanceof
NullAuthProvider)) {
+ _authProvider = runtimeProvider;
+ } else {
+ _authProvider =
AuthProviderUtils.makeAuthProvider(configs.get(MinionConstants.AUTH_TOKEN));
Review Comment:
The condition structure in `SegmentUploadContext` differs from the other
three locations (`authProvider == null || authProvider instanceof
NullAuthProvider` vs `runtimeProvider != null && !(runtimeProvider instanceof
NullAuthProvider)`). While logically equivalent, this inconsistency makes the
code harder to maintain. Use the same condition pattern across all four
locations for consistency.
```suggestion
if (runtimeProvider == null || runtimeProvider instanceof
NullAuthProvider) {
_authProvider =
AuthProviderUtils.makeAuthProvider(configs.get(MinionConstants.AUTH_TOKEN));
} else {
_authProvider = runtimeProvider;
```
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java:
##########
@@ -81,7 +82,11 @@ public SegmentConversionResult executeTask(PinotTaskConfig
pinotTaskConfig)
String downloadURL = configs.get(MinionConstants.DOWNLOAD_URL_KEY);
String uploadURL = configs.get(MinionConstants.UPLOAD_URL_KEY);
String originalSegmentCrc =
configs.get(MinionConstants.ORIGINAL_SEGMENT_CRC_KEY);
- AuthProvider authProvider =
AuthProviderUtils.makeAuthProvider(configs.get(MinionConstants.AUTH_TOKEN));
+ // Prefer the runtime minion AuthProvider; fallback to token if needed.
+ AuthProvider authProvider = MINION_CONTEXT.getTaskAuthProvider();
+ if (authProvider == null || authProvider instanceof NullAuthProvider) {
+ authProvider =
AuthProviderUtils.makeAuthProvider(configs.get(MinionConstants.AUTH_TOKEN));
+ }
Review Comment:
This auth provider resolution logic is duplicated in three places
(`BaseSingleSegmentConversionExecutor.executeTask`,
`BaseMultipleSegmentsConversionExecutor.preProcess`, and
`BaseMultipleSegmentsConversionExecutor.executeTask`). Consider extracting this
pattern into a shared helper method like `resolveAuthProvider(Map<String,
String> configs)` to reduce duplication and ensure consistent behavior across
all call sites.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
