This is an automated comment for commit 6e67edf with description of existing statuses. It's updated for the latest CI running

✅ Click here to open a full report in a separate page

The changes LGTM.

@Algunenano, what do you think about this? #58797

@Algunenano, what do you think about this? #58797

I'm planning on fixing it and hiding the object contents completely right after this PR. The idea is that the Settings objects will contain an array of SettingFieldBaseClass and most of the time we'd access a setting by calling getUInt64(settingRef, "string"). I prefer not exposing Settings::max_thread as that allows to hide the implementation completely, it's worse for IDEs but much better when we do any change. With this we could add a new setting, or change it's value/description, compile just one file, link and we're done.

For the other 1%, we could have special functions (we already have some here and there)

@Algunenano getUInt64(settingRef, "string") looks like not a good design, because you can easily make a typo and will debug it for hours. About Settings::max_thread: you will not expose it, it will be completely the same as ErrorCodes or CurrentMetrics, but you will need to export them everywhere.

@Algunenano getUInt64(settingRef, "string") looks like not a good design, because you can easily make a typo and will debug it for hours.
About Settings::max_thread: you will not expose it

We could do for something like (using MergeTreeSettings because it's easier to see):

• Call

#include <Storages/MergeTree/MergeTreeSettingGetter.h>

namespace MergeTreeSettings
{
extern const int replicated_max_parallel_fetches_for_table;
}

getInt64(settingRef, MergeTreeSettings::replicated_max_parallel_fetches_for_table)

• Storages/MergeTree/MergeTreeSettingGetter.h: Would just have the forward reference for MergeTreeSettingsStruct and for the getters.

Does that makes more sense?

Yes, exactly. This is what was written in Alexey's issue I suppose 🤣
But wait, why do we need to a forward reference? CurrentMetrics work without it. And you will never modify the header Settings.h afterwards, only Settings.cpp. Yes, it will be harder to develop something around Settings during that time, but will be Ok for the future.

But wait, why do we need to a forward reference? CurrentMetrics work without it.

We are not passing, copying and modifying CurrentMetrics objects all the time. If everyone knows about Settings, everyone needs to know about BaseSettings and SettingEnums, and again adding a new setting or a value might involve rebuilding thousands of files. Most of the users never hold an object, just a reference, so it's more natural to not expose it.

Another reason to prefer forward declarations: Many times when we use objects, it's by mistake: 56f497b. I should probably remove getSettingsRef(), make getSettings() return a const reference, and make explicit copies when needed.

Yes, I'm in favor of this change. Maybe rename getSettings to getSettingsCopy?