Skip to content

Add id info to config check utility#3141

Merged
EdColeman merged 2 commits into
apache:mainfrom
EdColeman:add_id_to_config_check
Jan 5, 2023
Merged

Add id info to config check utility#3141
EdColeman merged 2 commits into
apache:mainfrom
EdColeman:add_id_to_config_check

Conversation

@EdColeman
Copy link
Copy Markdown
Contributor

The ConfigCheckUtil.validate function logs issues with properties, but does not include an id to help find where the issue is occurring. This PR adds the namespace or table id - or if an id is not provided, it assumes the property is from the site configuration.

This was noticed after upgrading a 2.1 instance with replication configured. PR #3137 is independent of these changes, but this PR does provide additional information once upgrades are possible with PR #3137

Before the changes, the logs looks like:

2022-12-27T17:55:57,477 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.name)
2022-12-27T17:55:57,477 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.password.peer1)
2022-12-27T17:55:57,477 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.peer1)
2022-12-27T17:55:57,477 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.user.peer1)
2022-12-27T17:55:57,485 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.name)
2022-12-27T17:55:57,485 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.password.peer1)
2022-12-27T17:55:57,485 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.peer1)
2022-12-27T17:55:57,485 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.user.peer1)

With these changes:

2022-12-27T22:15:30,945 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.name) for site

2022-12-27T18:49:40,286 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.name) for +accumulo
2022-12-27T18:49:40,286 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.password.peer1) for +accumulo
2022-12-27T18:49:40,286 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.peer1) for +accumulo
2022-12-27T18:49:40,286 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.user.peer1) for +accumulo
2022-12-27T18:49:40,293 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.name) for +r
2022-12-27T18:49:40,293 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.password.peer1) for +r
2022-12-27T18:49:40,293 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.peer1) for +r
2022-12-27T18:49:40,293 [conf.ConfigCheckUtil] WARN : BAD CONFIG unrecognized property key (replication.peer.user.peer1) for +r
2022-12-27T18:49:40,299 [balancer.TableLoadBalancer] INFO : Loaded class org.apache.accumulo.core.spi.balancer.SimpleLoadBalancer for table +r

@EdColeman
Copy link
Copy Markdown
Contributor Author

EdColeman commented Jan 4, 2023

A question before merging - When passing the ID, if it is not namespace or table, then I pass null and assume that means system config for the log statement. General question is that correct or would something other than system be better displayed in the log statement.

@dlmarion
Copy link
Copy Markdown
Contributor

dlmarion commented Jan 4, 2023

So, given the levels of configuration (default, system, site, namespace, table), the default configuration can't be invalid. Is there a way to differentiate between the system and site configs?

Edit: Looking at the code, you might need to call ConfigCheckUtil.validate in the SystemConfiguration constructor passing in getSnapshot().

@EdColeman
Copy link
Copy Markdown
Contributor Author

So, given the levels of configuration (default, system, site, namespace, table)

I don't think so without major changes (and maybe not then) - but in this context I think that site, namespace and table are the only ones that matter - that is, they are the only ones users can control. Default and system are our responsibility and need to be correct in the code - this change is to log the id's so that users can see where BAD CONFIG errors are occurring.

@EdColeman
Copy link
Copy Markdown
Contributor Author

The code could be modified to pass a string instead of Id to allow more flexibility,

@EdColeman EdColeman merged commit 2d90a5c into apache:main Jan 5, 2023
@EdColeman EdColeman deleted the add_id_to_config_check branch January 5, 2023 14:06
deleteWatcher);
var conf = new NamespaceConfiguration(context, namespaceId, getSystemConfiguration());
ConfigCheckUtil.validate(conf);
ConfigCheckUtil.validate(conf, namespaceId.toString());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are strings, I think I would have preferred something like:

ConfigCheckUtil.validate(conf, "Namespace id: " + namespaceId);

So we know it's a namespace and not a table ID. Same with the table IDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants