Tablename-specific properties overridden in different table contexts

[hlgp]

Describe the bug Individual table-specific properties are able to be overridden in different table contexts with no clear indication of precedence. For example, table.custom.balancer.host.regex.accumulo.metadata can be set to different values on every table. In https://github.com/apache/accumulo/blob/main/server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java#L112 it gathers all table properties and basically the last one it reads is the one used.

To Reproduce set the accumulo metadata table regex to different values under different table contexts and see which is chosen in the balancer log message.

Expected behavior One would expect, given the tablename in the property, that the namesake table setting would take precedence followed by the system level. It seems odd from a design point of view to be able to override values within a table context and yet have properties with table names. If each table has table.custom.balancer.host.regex (sans tablename) set within its context, that would make sense, but the ability to override something as critical as the accumulo.metadata regex under myCrummyTestTable settings seems off.

[dlmarion]

table.custom.balancer.host.regex.accumulo.metadata should only be set once. I realize that the table properties can be specified per-table, but that's not how the code uses the properties. See the test[1] for an example. Maybe the properties should be renamed.

[1] https://github.com/apache/accumulo/blob/main/server/base/src/test/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancerTest.java

[dlmarion]

Specifically, these examples:

https://github.com/apache/accumulo/blob/12395eeec06bc88d9a530ccc9927d924eb8108e8/server/base/src/test/java/org/apache/accumulo/server/master/balancer/BaseHostRegexTableLoadBalancerTest.java#L78

https://github.com/apache/accumulo/blob/12395eeec06bc88d9a530ccc9927d924eb8108e8/server/base/src/test/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancerTest.java#L190

https://github.com/apache/accumulo/blob/12395eeec06bc88d9a530ccc9927d924eb8108e8/server/base/src/test/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancerTest.java#L246

[hlgp]

Yes, I recognize how the code is using the properties and I'm saying from a design standpoint it's too prone to error. If a property is meant to be set only once, the product should do something to enforce that or warn about it. In this case renaming the properties to omit the tablenames and simply defining it under the table context would work.

[dlmarion]

Understood. I didn't know if you were having an operational problem and I wanted to provide some feedback that might help you resolve the issue. I agree that a change may be needed here.

In this case renaming the properties to omit the tablenames and simply defining it under the table context would work.

Since the HostRegexTableLoadBalancer is meant to be configured as the master balancer, it might make more sense to create master.balancer.* prefix property and add them there.

[hlgp]

Understood. Such feedback is always welcome. We figured it out a day or two ago and nuked the setting on every table, leaving it only at the system level, but it was not intuitive while hunting it down. (No idea how they got set there, but still)

Yes, something like that would be good alternative as well.

[ivakegg]

I think the answer here is to simply put the property into the map in HostRegexTableBalancer.parseConfiguration IFF the table name from the property matches that of the table configuration being used. This should handle properties being set at the system level appropriately as well.

[hlgp]

@ivakegg I think that is a good first step for now. If the main reason the tablename properties came about was to be able to see them quickly at the system level, then I would argue that this is best addressed by a change to the config command. It would be ideal to be able to run something like a config -f regex --all and return all table settings so we don't have to dump all the configs to find them, keeping the table setting under table context paradigm.

[ctubbsii]

Based on the description, it seems that this issue is about how table-specific properties can override system-properties. However that is the design of per-table properties and works as expected.

Based on the discussion, though, it seems that this issue is not about table-specific properties overriding system properties, but specifically about the issues of HostRegexTableLoadBalancer poor use of properties. It should be mentioned that while these properties can be set at the per-table level (because of the property names and for historical reasons), this balancer cannot be used as a per-table balancer, and can only be instantiated at the main balancer (it has no per-table constructor that accepts a table id). The current code in the main branch was modified in #1201 (for target release 2.1.0) and never uses the per-table configuration for anything other than issue a warning. So, the only settings that will apply to this balancer are those placed on the system-wide configuration.

Given the state of things in the main branch, I don't believe there's anything more to be done here, other than to advise people to use this balancer very carefully... with the understanding that the properties should be set at the system level, in spite of their names (for historical reasons) being prefixed with table.

Please re-open or continue the discussion if there's more to address, but I think the fix in #1201 should address the concerns listed here already.

[hlgp]

Based on the description, it seems that this issue is about how table-specific properties can override system-properties. However that is the design of per-table properties and works as expected.

No, that is not what the issue is saying. I tried to clarify it in the discussion but it seems further clarity is needed.
Given a set of Tables T{T1, T2, ..., TN} and a set of configs C{C1,C2, ..., CN}, with some number of system-level properties S with C1, C2, ... CN set, for a given table T1 with C1,..., CN set, T1C1 can and does take precedence over SC1. This is good, right, and proper. The issue is (was) that there exists a set of properties with tablenames which can be set at both the system level and table level for each table, C.Tname. When I say "Individual table-specific properties are able to be overridden in different table contexts" I mean C.T1 could be overridden in T2 ( different tables meaning T1 != T2 ) in such a way that T1C.T1 and SC.T1 are ignored in favor of T2C.T1. C.T1 should be specific to T1 and no table T not equal to T1 should be able to override it. It sounds like #1201 addressed this so that SC.T1 would take precedence over all TC.T1 which fixes the specific issue with the HostRegexBalancer, but that violates the original idea that Tc should override Sc for all c in C.

[ctubbsii]

in such a way that T1C.T1 and SC.T1 are ignored in favor of T2C.T1

@hlgp I'm a little confused by your nomenclature, so let me translate first to make sure I understand, then I'll address it:

Imagine a config key: table.custom.table1.key

• In the system configuration, the value is set to v0
• In table1's configuration, the value is set to v1
• In table2's configuration, the value is set to v2
• In table3's configuration, no value is set

It sounds like what you're saying is that if table2 sets table.custom.table1.key = v2, then operations performing in the context of table1 will see the v2 value instead of v0 or v1. Is that what you are saying?

If so, that is definitely not the case. Operations performing in the context of table1 will see v1, operations performing in the context of table2 will see v2, and operations performing in the context of table3 will see v0.

The fact that the key contains the name of table1 has no bearing on whether its value will be seen in any particular context. In general, the only thing that matters is where the configuration is set.

The bug with the HostRegexTableLoadBalancer was a special case. It is not the case that the configuration of one table was affecting the context of another table. Rather, it was because that balancer was explicitly switching to each table's context and incorporating any config it found anywhere. The config wasn't affecting other contexts... the balancer was reading from all contexts. The current behavior fixes that bug, and only reads from the system context. Additionally, it logs warnings if it finds these properties in any per-table context, so that if something was working before and isn't after an upgrade, users will have some clue as to why.

This balancer should never have used the table.custom. prefix, as these properties were never intended to be stored in per-table configuration. They were intended to be stored in system configuration only... like other properties with the general.custom. prefix. I suspect that these predated the addition of general.custom. properties, and that's why it worked the way it did. This bug persists in 1.10, but the use of this balancer is a pretty niche use case, and there is a workaround. We could implement a simple fix in 1.10, like @ivakegg suggested, but nobody has done that. It would be a slight behavior change that could unexpectedly break people (while fixing others), so I'm not sure if it's worth it in a bugfix release.

So, I hope I understood what you were saying, and that my explanation makes sense. My main point is that I think this was an issue of HostRegexTableLoadBalancer, specifically, and not a general problem. Furthermore, it has been fixed in 2.1 development, and there is a workaround in 1.10. If somebody submits a PR to fix it for 1.10, feel free to ping me on it and I'd be happy to review it. I think it might just involve adding a line to if (!tableName.equals(table.getKey())) { continue; } after https://github.com/apache/accumulo/blob/56142a89952533fef922fa86739a879c073e7c2a/server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java#L244 However, I have not experimented or tested with this. It's just a guess from glancing at the code, and would probably need some sort of unit test case to go with it.

[hlgp]

"It sounds like what you're saying is that if table2 sets table.custom.table1.key = v2, then operations performing in the context of table1 will see the v2 value instead of v0 or v1. Is that what you are saying?" -- No, I was not talking about operations within the context of table1. I was talking about system-level operations, as exhibited by the HostRegexTabletLoadBalancer which you characterize perfectly here. Everything hereafter in your synopsis I agree with. We are saying the same things. The fix is great for this specific issue, but I was thinking bigger picture thereafter. I keep getting told what the code does and I'm trying to propose what it could do.

What I was saying further was that the behavior of the HostRegexTabletLoadBalancer brought into question the need for system-level tablename properties (config.foo.bar.table1) since it does (or did at the time) pull from all contexts and could have surmised, from each table's context, that tables own regex. Accumulo has a very nice config inheritance structure: system-level configs inherited or overridden within each table context as necessary. Having a table-name-specific-system-level property blurs that structure in a way that I thought it was worth exploring a refactor. If in 2.0+ the system-level operations no longer have access to the table contexts, then perhaps this point is moot. If not, it was worth opening up discussions for a streamlining redesign to keep all table-specific overrides solely within that tables context.

[ctubbsii]

I keep getting told what the code does and I'm trying to propose what it could do.

Sorry about that! I understand now.

If in 2.0+ the system-level operations no longer have access to the table contexts, then perhaps this point is moot.

In 2.0+, these kinds of operations can still access per-table config, if they need it. Even if we didn't provide an API through the pluggable interface, like we do for the balancer right now, they could always use some other tactic (like reflection or accessing static APIs) to get what they want. So, I don't see that going away any time soon.

Having a table-name-specific-system-level property blurs that structure in a way that I thought it was worth exploring a refactor.

I'm not sure if this would be worth it.

In general, I've become frustrated with the restrictions of our "we establish the schema, you take what you get" model of configuration property naming. It leads to pluggable implementations that ship with Accumulo using general.custom.* and table.custom.*, and implementations that could be modularized and separated from our main code base using their own dedicated property keys listed in Property.java to the point where they are too tightly coupled with other Accumulo code. It prevents a company at, say "Example, Inc." from using the property namespace com.example.awesomeiterator.feature1=enabled, and forces too verbose property names for ones we maintain ourselves.

Rather than create a new dedicated namespace for table configuration properties for use in the system namespace, I'd rather relax the restrictions on property names. Then, we could do something like hostregexbalancer.regex.table1=... and not be so controlling of how properties are set and managed. We can reserve the accumulo.* namespace for ourselves, of course (and the current namespaces, system.*, general.*, table.*, etc. for backwards-compatibility), but otherwise, I don't think we should be too restrictive about properties users set, whether they are in system config or per-table config.

[hlgp]

Oh, yes, I entirely agree with keeping properties flexible, but keeping to certain tenets can also prevent confusion (like what we just experienced with the balancer) and unintended or obscured contradictions in configs. Maintaining the simple construct of "if you want this at the system level, set it thus" and "if you want this on specific tables, set it thus" while minimizing possible contradictions and still allowing for custom properties at both levels would be the goal.

[ctubbsii]

Maintaining the simple construct of "if you want this at the system level, set it thus" and "if you want this on specific tables, set it thus" while minimizing possible contradictions and still allowing for custom properties at both levels would be the goal.

Well, we do have that separation today (even for 1.10; general.* vs. table.*). HostRegexTableLoadBalancer was just not following it, and instead did it's own thing. Convention is one thing. Enforcement is harder. :smiley_cat: