helix icon indicating copy to clipboard operation
helix copied to clipboard

Are all resources expected to have ResourceConfig?

Open hmhagberg opened this issue 4 years ago • 5 comments

ConfigAccessor#getResourceConfig logs warning (and returns null) if config for some resource is missing from ZooKeeper. We are seeing quite a few warnings about missing resource configs due to this. However, the system appears to be working correctly regardless. Example warning:

2022-04-08 10:45:38,563 +0200 WARN  [ConfigAccessor           ] - No config found at /foo-cluster/CONFIGS/RESOURCE/bar-resource

Are all resources expected to have ResourceConfig? If not, the warning appears to be invalid and should be removed or lowered to info. If yes, it would be great if either an empty ResourceConfig was automatically created or requirement to set ResourceConfig was reflected in documentation and HelixAdmin API.

hmhagberg avatar Apr 08 '22 13:04 hmhagberg

Hi @hmhagberg - thank you for the question. The historical context is this: IdealState is the de-facto definition for a resource with its required fields, but applications wanted to add more application-specific configs. But the original intention was to make IdealStates immutable (not enforced, unfortunately), so Helix added support for ResourceConfig. This is why things can work without ResourceConfig, but the intention is for a given resource to have a ResourceConfig and an IdealState pair.

But the log message seems to be coming from ConfigAccessor, which is entirely used by the application logic. Are you trying to ConfigAccessor#get? This, I don't believe, is being called by the Helix controller pipeline.

narendly avatar Apr 08 '22 13:04 narendly

@narendly thanks for the quick reply! We are not using ConfigAccessor in our application logic. However, I was wrong about the warning coming from getResourceConfig -- instead it comes from get which is called with scope RESOURCE in HelixTaskExecutor here.

the intention is for a given resource to have a ResourceConfig and an IdealState pair

Good info! So the warning is indeed valid and we should be creating ResourceConfig along with IdealState.

Do you think it would make sense to create ResourceConfig by default in HelixAdmin#addResource?

hmhagberg avatar Apr 08 '22 15:04 hmhagberg

@hmhagberg Thanks for explaining where the warn log was coming from.

Good info! So the warning is indeed valid and we should be creating ResourceConfig along with IdealState. Actually, that's not what I meant - that is only so if the application needs to encode more configs (other than what's required in IdealState). In such cases, a ResourceConfig must be created and the application should add the configs in ResourceConfig, not IdealState.

If there aren't such extra configs, then ResourceConfig is optional.

But in your case though, it seems that you are using dedicated thread pools for your resources? If so, are you creating a proper ResourceConfig and adding a thread pool size field to it?

narendly avatar Apr 08 '22 15:04 narendly

@narendly alright, in that case we should be good with just IdealState since we aren't actually using dedicated thread pools. My understanding of the updateStateTransitionMessageThreadPool method is that its purpose is to check if dedicated thread pool has been configured for a resource. Hence it's a bit confusing to get a warning since our intention is not to use one.

You can quite easily reproduce the warning by running TestRestartParticipant test (you need to change level of org.apache logger first).

69335 [ClusterManager_Watcher_TestRestartParticipant_localhost_12918_PARTICIPANT_10001] WARN  org.apache.helix.ConfigAccessor [] - No config found at /TestRestartParticipant/CONFIGS/RESOURCE/TestDB0.

hmhagberg avatar Apr 11 '22 09:04 hmhagberg

@hmhagberg we can have someone look further into this issue. If it is indeed the case that we're unnecessarily emitting logs then that'd need fixing.

narendly avatar Apr 11 '22 17:04 narendly