[GitHub] [pinot] gortiz commented on pull request #10352: Encapsulate changes in IndexLoadingConfig and SegmentGeneratorConfig

["gortiz (via GitHub)" <gi...@apache.org>]

gortiz commented on PR #10352:
URL: https://github.com/apache/pinot/pull/10352#issuecomment-1449501930

   > Trying to understand why making the IndexLoadingConfig fields immutable can help here. The fields within IndexLoadingConfig is already a copy of the original config, so it should be okay to modify them.
   
   It is mandatory in index-spi, where we assume that index config is always going to be read from a `TableConfig`. What have to do there is different in `IndexLoadingConfig` and `SegmentGeneratorConfig` because they are used in a different way.
   
   In `IndexLoadingConfig` case we needed to add a new method that transform the internal state of `IndexLoadingConfig` into a config object. But to know when we need to call that method we introduced a boolean attribute called `dirty` (in index-spi). That attribute is changed whenever a mutator method is called in `IndexLoadingConfig`. This works... unless tests actually modify the mutable collections of private attributes in `IndexLoadingConfig`, breaking the encapsulation. Here we have two options:
   - To forbid external mutation of private attributes (which improves the encapsulation)
   - To add some _barriers_ in the code. These would be special points when we know that the `IndexLoadingClass` will not be modified again, so we can safely calculate there the index config object  there. This approach is less resilient. If we don't detect one of these _barriers_, then the result is not correct.
   
   I don't remember exactly why, but in `SegmentGeneratorConfig` we couldn't use these techniques, so instead of doing that, `SegmentGeneratorConfig` uses another technique where each time a mutator method is called, the index config is recalculated. This end up being a cleaner solution, but also a bit more expensive (as it needs to create several objects). I may try to implement this solution in `IndexLoadingConfig` (as suggested in the PEP) but both solutions require to remove the external mutation of internal attributes.
   
   > Ideally we want to make IndexLoadingConfig directly read config from TableConfig and Schema
   
   Yes. I know, but meanwhile we need a solution to the external mutation problem. This code is already included in index-spi, but I think that it would be better to add it in a different PR in order to:
   - Reduce the complexity of index-spi. This feature is something orthogonal to index-spi. We shouldn't break the encapsulation of our objects.
   - Forbid new tests created before index-spi is merged to use internal mutability (and therefore make easier to fix conflicts in index-spi).
   


-- 
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