ctubbsii commented on a change in pull request #2143:
URL: https://github.com/apache/accumulo/pull/2143#discussion_r645044101
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1045,40 +1045,44 @@
"The maximum size of a message that can be sent to a tablet server."),
// CompactionCoordinator properties
@Experimental
- COORDINATOR_PREFIX("coordinator.", null, PropertyType.PREFIX,
+ COMPACTION_COORDINATOR_PREFIX("compaction_coordinator.", null,
PropertyType.PREFIX,
Review comment:
For a long time, I've been thinking about making the keys to these
properties derived from the ENUM name, with some simple rules:
```java
String key = en.replaceAll("_", ".").toLowerCase();
```
That hasn't been feasible at this time, because there's currently too many
properties that break that rule, and I haven't gone in to do the work to rename
the enums to match the keys, or worked out the kinks. However, I do like to see
consistency between the naming of the enum and the key, so we, as developers,
can easily find / recognize the properties and enums as being related, when we
are looking at code or config that has one or the other. After all, it would be
*very* confusing if the enum had little to do with the key.
So, having said all that, it's a bit weird to see an underscore in the key
name, since in my mind, that's a different naming convention. It throws a
wrench in the general naming conventions for underscores in enum names that map
to dots in keys.
A dash might make more sense, because this is effectively a compound
adjective describing a server (as in "a compaction-coordinator server"), but
there's not an equivalent enum name. Omitting the underscore would result in
`COMPACTIONCOORDINATOR_PREFIX("compactioncoordinator.", ...`, though.
We do have some precedent for mixed case (`useJsse`, `keyStore`,
`permissionHandler`, `clientAuth`, etc.). These should just be for presentation
convenience, as actual parsing of the keys should be case-insensitive (I'm not
sure if that's actually the case, though... but that would be a separate
issue). If you follow that convention, it would be `compactionCoordinator.`,
which isn't bad.
If you go with dots, then you could go with `compaction.coordinator.` and
`compaction.worker.`, since they come as a set.
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1045,40 +1045,44 @@
"The maximum size of a message that can be sent to a tablet server."),
// CompactionCoordinator properties
@Experimental
- COORDINATOR_PREFIX("coordinator.", null, PropertyType.PREFIX,
+ COMPACTION_COORDINATOR_PREFIX("compaction_coordinator.", null,
PropertyType.PREFIX,
Review comment:
I like `compaction.coordinator` with `compaction.worker` better than I
like `compaction-coordinator` and `compactor`, and I like that one slightly
better than `compactionCoordinator` and `compactor`.
You're right there's no precedent for use of dash for property keys in our
code. Additionally, a quick Google search for dashes in config keys resulted in
several discussions about constraints and even some bugs in Spring, ConfigJSR
(JSR-382), and other configuration systems. One of the popular issues was that
some of these systems try provide ways to use configuration properties as
environment variables, and you can't use dashes in environment variables in
Linux. Those projects seemed to try workarounds that involved fuzzy matching,
but those solutions struggled with internationalization issues. For us, I think
it's probably just best to avoid dashes in our keys, to ensure flexibility in
future for our configuration keys, and to avoid pitfalls that others have
experienced.
Dots seem to be the preferred delimiter for config files. Underscores seem
to be borrowing from a different naming convention. I would stick with dots or
none.
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1045,40 +1045,44 @@
"The maximum size of a message that can be sent to a tablet server."),
// CompactionCoordinator properties
@Experimental
- COORDINATOR_PREFIX("coordinator.", null, PropertyType.PREFIX,
+ COMPACTION_COORDINATOR_PREFIX("compaction_coordinator.", null,
PropertyType.PREFIX,
Review comment:
> I am under the impression (maybe mistakenly) that the first part of
the property name for processes should be the process name (`manager`, `gc`,
`tserver`, `monitor`, etc).
Roughly speaking, properties are organized that way, but we also have other
prefixes `rpc`, `instance`, `general`, `table`, and `replication. For these, I
sort of see the compaction coordinator and the compactors as part of a single
external system (prefix `compaction`) with two sub-components of that
externalized system that are servers (performing the roles of `coordinator` and
`worker`).
The important part isn't that the first part be a server type, but that
properties are organized so they can be scoped to a particular component. In
other words, we can exclude all `table.*`, and `gc.*` if we're starting a
tserver. In theory, configuration could even be spread across different config
files (as in `bin/accumulo tserver --config /path/to/tserver.conf`, where
`tserver.conf` contains some `include = instance.props` directive and a bunch
of `tserver.<prop> = <value>` lines). So, organization is important to support
various configuration management use cases, but having the first element be a
server type isn't specifically important.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
