somandal commented on code in PR #17159:
URL: https://github.com/apache/pinot/pull/17159#discussion_r2504882144
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/ServerReloadJobStatusCache.java:
##########
@@ -35,26 +42,33 @@
*
* <p>Thread-safe for concurrent access. Uses Guava Cache with LRU eviction
* and time-based expiration.
+ *
+ * <p>Implements PinotClusterConfigChangeListener to support dynamic
configuration
+ * updates from ZooKeeper cluster config. When config changes, cache is rebuilt
+ * with new settings and existing entries are migrated.
*/
@ThreadSafe
-public class ServerReloadJobStatusCache {
+public class ServerReloadJobStatusCache implements
PinotClusterConfigChangeListener {
private static final Logger LOG =
LoggerFactory.getLogger(ServerReloadJobStatusCache.class);
+ private static final String CONFIG_PREFIX =
"pinot.server.table.reload.status.cache";
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
- private final Cache<String, ReloadJobStatus> _cache;
+ private volatile Cache<String, ReloadJobStatus> _cache;
+ private ServerReloadJobStatusCacheConfig _currentConfig;
/**
- * Creates a cache with the given configuration.
- *
+ * Creates a cache with default configuration.
Review Comment:
i would not consider this comment resolved.
These are not orthogonal configs anyways, they're the same config - just
picked up from different paths. I can provide examples if you'd like as we do
this extensively throughout Pinot code. Check out how we initialize
`SegmentDownloadThrottler` in the server starter. We don't hardcode, instead we
check if a config exists and use that to initialize or fallback to a default.
IMO we do need to address this. I know that some companies outside StarTree
cannot use cluster configs to override at all, and use other mechanisms such as
server configurations separated out by tenants. If we are adding functionality
IMO we should ensure we support it for all deployments in OSS since this code
will live in OSS. So adding the correct mechanism to provide defaults is a must
have, and the ability to override defaults via cluster level configs without
requiring restarts is a nice to have. Without addressing this, deployments that
cannot use cluster configs cannot override the hardcoded defaults at all.
I can provide more context on this if you'd like, but Pinot is infra and we
need to learn how configs are handled and support them correctly until we have
a better solution.
Also let's get this PR reviewed by @Jackie-Jiang as well to ensure he's
onboard with whatever decision is taken.
cc @Jackie-Jiang
--
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]
