You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "suvodeep-pyne (via GitHub)" <gi...@apache.org> on 2025/09/15 22:11:02 UTC
[PR] [audit] Extend audit logging support to broker REST APIs [pinot]
suvodeep-pyne opened a new pull request, #16823:
URL: https://github.com/apache/pinot/pull/16823
## Summary
This PR extends the existing audit logging infrastructure from the controller to support broker REST APIs. The implementation follows the same pattern used in the controller, enabling audit logging for broker query and admin endpoints.
## Changes
### 1. BrokerAdminApiApplication.java
- Added AuditLogFilter registration to intercept Jersey requests for audit logging
- Filter is registered alongside other filters like AuthenticationFilter
### 2. BaseBrokerStarter.java
- Added DefaultClusterConfigChangeHandler field to handle dynamic cluster configuration updates
- Registered the config change handler with Helix manager for real-time config updates
- Integrated AuditServiceBinder in createBrokerAdminApp() to wire up audit dependencies
- Marked existing _clusterConfigChangeHandlers as deprecated in favor of dedicated config handler
## Design Decisions
- **Direct Helix Registration**: Following the controller pattern, the DefaultClusterConfigChangeHandler is registered directly with the Helix manager rather than through broker's ClusterChangeHandler infrastructure
- **Reuse Existing Infrastructure**: All audit components from pinot-common are reused without modification
- **Service-specific Configuration**: Broker uses pinot.audit.broker.* configuration prefix, separate from controller
## Configuration
To enable audit logging for broker, add these properties to cluster configuration:
```properties
pinot.audit.broker.enabled=true
pinot.audit.broker.capture.request.payload.enabled=true
pinot.audit.broker.request.payload.size.max.bytes=8192
pinot.audit.broker.url.filter.include.patterns=/query.*,/routing.*
pinot.audit.broker.capture.request.headers=Authorization,X-Request-ID
```
## Testing
- Compilation verified for both pinot-common and pinot-broker modules
- Audit logging is disabled by default, ensuring no impact on existing deployments
- Configuration can be updated dynamically via cluster config without restart
## Impact
- **Backward Compatible**: No changes to existing APIs or behavior when disabled
- **Performance**: Minimal overhead when disabled, configurable sampling for high-volume endpoints
- **Security**: Enables compliance and security auditing for broker operations
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2350316529
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
Yes. This is being used by others currently. This is more like a signal. Marking them `@Deprecated` is a way to ensure more folks don't follow the same trend and provides clear direction on which one to use given both options. This in a way helps the next person working on this to not wonder which way to go forward.
```
protected final List<ClusterChangeHandler> _clusterConfigChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _idealStateChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _externalViewChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _instanceConfigChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _liveInstanceChangeHandlers = new ArrayList<>();
```
The current design uses 5 different `ClusterChangeHandler` lists to manage different kinds of use.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal merged PR #16823:
URL: https://github.com/apache/pinot/pull/16823
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16823:
URL: https://github.com/apache/pinot/pull/16823#issuecomment-3294255262
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16823?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 56.32%. Comparing base ([`c9e2bf2`](https://app.codecov.io/gh/apache/pinot/commit/c9e2bf26e82deab32176ea55b0d2eb20c5076978?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`04c4933`](https://app.codecov.io/gh/apache/pinot/commit/04c4933d11102a17521afb8a25e097e06e8bc5ca?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (c9e2bf2) and HEAD (04c4933). Click for more details.
>
> <details><summary>HEAD has 28 uploads less than BASE</summary>
>
>| Flag | BASE (c9e2bf2) | HEAD (04c4933) |
>|------|------|------|
>|java-21|5|1|
>|unittests|4|2|
>|temurin|9|2|
>|java-11|4|1|
>|unittests2|2|0|
>|integration|5|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16823 +/- ##
============================================
- Coverage 63.39% 56.32% -7.08%
+ Complexity 1400 701 -699
============================================
Files 3057 2396 -661
Lines 179191 133196 -45995
Branches 27456 21191 -6265
============================================
- Hits 113604 75017 -38587
+ Misses 56821 51930 -4891
+ Partials 8766 6249 -2517
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.27% <ø> (-7.07%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.27% <ø> (-7.08%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.32% <ø> (-7.08%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.32% <ø> (-7.08%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.32% <ø> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16823/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16823?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2353216452
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
Thanks a lot for your diligence on this, I really appreciate the careful review. 🙏 You’re absolutely right that the `ClusterChangeHandler` serves broader use cases and brings additional capabilities like periodic syncs that are valuable. I don’t want to take away from that at all.
I spent some time when deciding on whether to reuse the existing infra. Where I’m coming from is more specific to the `BaseBrokerStarter._clusterConfigChangeHandlers` list. My thinking is:
* **Centralized handling** – The `ClusterConfigChangeListener` approach consolidates how cluster config changes are surfaced across controller, server, and now broker, giving us a unified way to process them.
* **Dedicated interface** – It provides a clear, dedicated contract around changed components and configs, which makes it easier for future contributors to know upfront *what exactly has changed*.
* **Container for evolution** – Because it’s scoped specifically to cluster config changes (rather than Helix changes in general), it can evolve independently as we add new requirements.
* **Avoiding duplication** – Having both `_clusterConfigChangeHandlers` and `_defaultClusterConfigChangeHandler` maintain parallel lists essentially duplicates functionality. Consolidating into one prevents future contributors from having to guess which path is the “right” one.
That’s why I suggested deprecating the list inside `BaseBrokerStarter`—not the `ClusterChangeHandler` itself. In fact, I see value in keeping `ClusterChangeHandler` for more general use cases around listening to cluster "changes".
I completely hear your concern that we shouldn’t close off the `ClusterChangeHandler` option prematurely. My view is that deprecating the list in `BaseBrokerStarter` helps provide a clear instruction for new code while still allowing folks to use `ClusterChangeHandler` where its broader functionality is actually needed.
Happy to discuss on a call if you feel integrating the Audit code with the existing mechanism is the better short-term compromise. My concern is that leaving both lists side by side will make it harder for the next person to know the preferred path forward.
What do you think?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on PR #16823:
URL: https://github.com/apache/pinot/pull/16823#issuecomment-3294318091
> just making sure, was this change tested out using a Quickstart or something? also, don't see any new unit tests added, is that going to be a follow-up PR?
Yes. This was manually tested with quickstart. Also, in this PR we aren't really adding any new business logic but rather wiring up existing tested code with the broker.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2353116947
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
but doesn't `BaseBrokerStarter._clusterConfigChangeHandlers` use the `ClusterChangeHandler`. why are we making a call that this is not the preferred way for the handling cluster config changes? that's my question - we should eventually consolidate the two mechanisms, rather than marking this list as deprecated. and like I mentioned they are both meant to update the configs but do it in different ways with additional capabilities in the older way.
I don't think that we should expect folks to stop using the `ClusterChangeHandler` way of handling the changes for all cluster configs going forward. that may be more desirable for some use cases especially for where's it's used on the Broker side today (or at least i haven't done the analysis for whether the additional functionality is necessary or not).
Perhaps you can instead integrate your Audit code to use the existing mechanism and use `ClusterChangeHandler` if you want to have a single way to do this on the Broker side?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2353088183
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
So we're not deprecating the `ClusterChangeHandler ` here. It seems to serve a more general use case as you also pointed out. The current proposal is to deprecating specifically the `_clusterConfigChangeHandlers` list in favor of the `DefaultClusterConfigChangeHandler` instance that can maintain a list of registrants within it. Note that both do essentially the same thing and that's why I think we can consolidate.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on PR #16823:
URL: https://github.com/apache/pinot/pull/16823#issuecomment-3304684455
@suvodeep-pyne you might want to rebase:
```
Comparing binary compatibility of /home/runner/work/pinot/pinot/pinot-spi/target/pinot-spi-1.5.0-SNAPSHOT.jar against /home/runner/work/pinot/pinot/master-head/pinot-spi/target/pinot-spi-1.5.0-SNAPSHOT.jar
WARNING: You are using the option '--ignore-missing-classes', i.e. superclasses and interfaces that could not be found on the classpath are ignored. Hence changes caused by these superclasses and interfaces are not reflected in the output.
***! MODIFIED CLASS: PUBLIC STATIC org.apache.pinot.spi.utils.CommonConstants$Broker$Request$QueryOptionKey (not serializable)
=== CLASS FILE FORMAT VERSION: 55.0 <- 55.0
=== UNCHANGED SUPERCLASS: java.lang.Object (<- java.lang.Object)
---! REMOVED FIELD: PUBLIC(-) STATIC(-) FINAL(-) java.lang.String LITE_MODE_SERVER_STAGE_LIMIT
E: There is at least one incompatibility: org.apache.pinot.spi.utils.CommonConstants$Broker$Request$QueryOptionKey.LITE_MODE_SERVER_STAGE_LIMIT:FIELD_REMOVED
See '--help' or '-h' for more information.
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2356730178
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
Thanks @Jackie-Jiang @somandal
Updated the PR. Removed the deprecation and replaced it with a `TODO`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2350316529
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
Yes. This is being used by others currently. This is more like a signal. Marking them `@Deprecated` is a way to ensure more folks don't follow the same trend and provides clear direction on which one to use given both options.
```
protected final List<ClusterChangeHandler> _clusterConfigChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _idealStateChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _externalViewChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _instanceConfigChangeHandlers = new ArrayList<>();
protected final List<ClusterChangeHandler> _liveInstanceChangeHandlers = new ArrayList<>();
```
The current design uses 5 different `ClusterChangeHandler` lists to manage different kinds of use.
I think being decisive at this time helps make the decision for others going forward as we plan to refactor integrate all cluster config handling to a central class.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2352837629
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
I don't think deprecating the existing ClusterChangeHandler is the right long-term approach. I would prefer if we consolidated everything going forward instead. The existing ClusterChangeMediator / ClusterChangeHandler do many other things like periodically syncing the configs in case Helix callback is missed, etc, which could be useful to have.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2350290048
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
why deprecate this? this is used for other cluster config change handlers, right?
I would recommend adding a TODO to consolidate the approach to a single one instead
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2353088183
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
So we're not deprecating the `ClusterChangeHandler ` here. It seems to serve a more general use case as you also pointed out. The current proposal is to deprecating specifically the `BaseBrokerStarter._clusterConfigChangeHandlers` list in favor of the `BaseBrokerStarter._defaultClusterConfigChangeHandler` instance that can maintain a list of registrants within it. Note that both do essentially the same thing and that's why I think we can consolidate.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "suvodeep-pyne (via GitHub)" <gi...@apache.org>.
suvodeep-pyne commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2353088183
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
So we're not deprecating the `ClusterChangeHandler ` here. It seems to serve a more general use case as you also pointed out. The current proposal is to deprecating specifically the `BaseBrokerStarter._clusterConfigChangeHandlers` list in favor of the `DefaultClusterConfigChangeHandler` instance that can maintain a list of registrants within it. Note that both do essentially the same thing and that's why I think we can consolidate.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [audit] Extend audit logging support to broker REST APIs [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16823:
URL: https://github.com/apache/pinot/pull/16823#discussion_r2356306487
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java:
##########
@@ -129,6 +131,12 @@ public abstract class BaseBrokerStarter implements ServiceStartable {
private volatile boolean _isStarting = false;
private volatile boolean _isShuttingDown = false;
+ // Class dedicated towards handling cluster config change
+ protected final DefaultClusterConfigChangeHandler _defaultClusterConfigChangeHandler =
+ new DefaultClusterConfigChangeHandler();
+
+ // Deprecated in favor of using a dedicated _defaultClusterConfigChangeHandler to manage config related changes
+ @Deprecated
Review Comment:
In the long term, we want to use `DefaultClusterConfigChangeHandler` to handle cluster config change because it helps extract the changed properties.
I'd suggest adding a TODO instead of deprecate annotation because it is not really deprecated. We need a separate refactor to migrate the list to `DefaultClusterConfigChangeHandler`, then we can mark it as deprecated
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org