ctubbsii commented on code in PR #3180:
URL: https://github.com/apache/accumulo/pull/3180#discussion_r1099164108
##########
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java:
##########
@@ -382,44 +382,37 @@ public short getDefaultReplication(Path path) {
*/
private static Configuration
getVolumeManagerConfiguration(AccumuloConfiguration conf,
final Configuration hadoopConf, final String filesystemURI) {
+
final Configuration volumeConfig = new Configuration(hadoopConf);
- final Map<String,String> customProps =
-
conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUMES_CONFIG);
- customProps.forEach((key, value) -> {
- if (key.startsWith(filesystemURI)) {
- String property = key.substring(filesystemURI.length() + 1);
- log.info("Overriding property {} to {} for volume {}", property,
value, filesystemURI);
- volumeConfig.set(property, value);
- }
- });
- return volumeConfig;
- }
- private static void warnVolumeOverridesMissingVolume(AccumuloConfiguration
conf,
- Set<String> definedVolumes) {
- final Map<String,String> overrideProperties = new ConcurrentHashMap<>(
-
conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUMES_CONFIG));
+
conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUMES_CONFIG).entrySet().stream()
+ .filter(e -> e.getKey().startsWith(filesystemURI + ".")).forEach(e -> {
+ String key = e.getKey().substring(filesystemURI.length() + 1);
+ String value = e.getValue();
+ log.info("Overriding property {} for volume {}", key, value,
filesystemURI);
+ volumeConfig.set(key, value);
+ });
- definedVolumes.forEach(vol -> {
- log.debug("Looking for defined volume: {}", vol);
- overrideProperties.keySet().forEach(override -> {
- if (override.startsWith(vol)) {
- log.debug("Found volume {}, removing property {}", vol, override);
- overrideProperties.remove(override);
- }
- });
- });
+ return volumeConfig;
+ }
- overrideProperties.forEach((k, v) -> log
- .warn("Found no matching volume for volume config override property {}
= {}", k, v));
+ protected static List<Entry<String,String>>
+ findVolumeOverridesMissingVolume(AccumuloConfiguration conf, Set<String>
definedVolumes) {
+ return
conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUMES_CONFIG).entrySet()
+ .stream()
+ // log only configs where none of the volumes (with a dot) prefix its
key
+ .filter(e -> definedVolumes.stream().noneMatch(vol ->
e.getKey().startsWith(vol + ".")))
+ .collect(Collectors.toList());
Review Comment:
I see you needed this as a separate method for verification in the test.
However, it's a bit weird to return a list of entries, rather than a map. But,
there's also a performance hit by collecting it into a data structure in the
first place. I don't think you need that at all for the test or for logging the
warning. Just return the Stream, which you can call forEach on directly for the
warning... and for the test you can check it as you stream, or call `.collect`
on it there to do whatever verification you need.
```suggestion
protected static Stream<Entry<String,String>>
findVolumeOverridesMissingVolume(AccumuloConfiguration conf,
Set<String> definedVolumes) {
return
conf.getAllPropertiesWithPrefixStripped(Property.INSTANCE_VOLUMES_CONFIG).entrySet()
.stream()
// log only configs where none of the volumes (with a dot) prefix
its key
.filter(e -> definedVolumes.stream().noneMatch(vol ->
e.getKey().startsWith(vol + ".")));
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
