vvivekiyer commented on pull request #8110: URL: https://github.com/apache/pinot/pull/8110#issuecomment-1028397694
> Should we make this feature enabled via a controller config (as opposed to http header setting)? I would imagine that the installations that need protection against new segments being pushed simultaneously (for whatever reason) will want to do so for all requests and all tables. I agree that making this a controller config would be ideal. But here are the concerns with doing that in the current implementation: 1. "enableParallelPushProtection" is an existing parameter (not derived from http header) in the REST API call. But it's being used only for existing segments. This patch extends that to work for new segments as well. That means that there could be some customers (scripts, curl commands, etc) using this API parameter currently. 2. Removing the existing "enableParallelPushProtection" parameter from the API is not possible (for backward compatibility) without deprecating the API. At the same time, having both controller config and a parameter will get hairy in terms of code-handling, readability and customer usage. Given these concerns, I feel that it would be better to address this as a TODO in the future as follows: 1. Deprecate existing UploadSegment APIs. Introduce new API without the parameter. 2. Introduce a controller config option and extend code to use the config to provide parallel push protection Thoughts? -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
