apache / pinot

Apache Pinot - A realtime distributed OLAP datastore
https://pinot.apache.org/
Apache License 2.0
5.31k stars 1.24k forks source link

`pinot.server.instance.reload.consumingSegment` deprecated? #7924

Open mapshen opened 2 years ago

mapshen commented 2 years ago

There are several references to pinot.server.instance.reload.consumingSegment in the docs such as [1] and [2]. When I go to the code base at [3], it seems CONFIG_OF_INSTANCE_RELOAD_CONSUMING_SEGMENT is never used anywhere.

[1] https://docs.pinot.apache.org/configuration-reference/server [2] https://docs.pinot.apache.org/users/tutorials/schema-evolution [3] https://github.com/apache/pinot/blob/12edbdb1e521e0d9ee35a2d6ef2fed172e5beaa1/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java#L254-L255

Jackie-Jiang commented 2 years ago

It is used in HelixInstanceDataManagerConfig.shouldReloadConsumingSegment(). There is no directly access to the constant because the prefix is trimmed off in the instance data manager config.

mapshen commented 2 years ago

It is used in HelixInstanceDataManagerConfig.shouldReloadConsumingSegment(). There is no directly access to the constant because the prefix is trimmed off in the instance data manager config.

As per [1], pinot.server.instance.reload.consumingSegment is set to false by default, but based on our tests, without setting this to true, the current consuming segment(s) will still reflect the default null value for newly added columns, which conflicts with what documented at [2]

Real-Time Pinot table: In case of real-time tables, make sure the "pinot.server.instance.reload.consumingSegment" config is set to true inside Server config. Without this, the current consuming segment(s) will not reflect the default null value for newly added columns.

[1] https://docs.pinot.apache.org/configuration-reference/server [2] https://docs.pinot.apache.org/users/tutorials/schema-evolution

Jackie-Jiang commented 2 years ago

Good catch! I think we forgot to update the documentation. The default is actually true (CommonConstants.Server.DEFAULT_RELOAD_CONSUMING_SEGMENT). Let me update the documentation.