Closes #806 - Added branch option to agent mappings #809
Conversation
There was a problem hiding this comment.
Reviewed 16 of 26 files at r1.
Reviewable status: 16 of 26 files reviewed, 19 unresolved discussions (waiting on @JonasKunz)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentconfiguration/AgentConfigurationManager.java, line 81 at r1 (raw file):
* * @param agentAttributes the attributes of the agent for which the configuration shall be queried *
Have you changed the formatting settings or why do we have these changes (line between return and parameter), now?
When I'm formatting the project, all these changes are rolled back to the previous state.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentconfiguration/AgentConfigurationReloadTask.java, line 57 at r1 (raw file):
fileManager.getWorkspaceRevision()
Can you store this in a local variable? otherwise it may be (very unlikely) that you get a different accessor three lines below.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/autocomplete/util/ConfigurationFilesCache.java, line 71 at r1 (raw file):
activeReloadTask.cancel(); } activeReloadTask = new ConfigurationFilesCacheReloadTask(fileAccess, (configs) -> currentParsedFiles = configs);
Instead of the CancellableTask you could use Java's CompletableFuture which could fit for your purpose:
Future<Void> future = CompletableFuture.supplyAsync(()->work).thenAccept((result)->callback);
future.cancel(false);
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 47 at r1 (raw file):
private VersioningManager versioningManager; private CachingRevisionAccess cachedLiveRevision;
Can you add a short doc to both fields
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 55 at r1 (raw file):
accidently
accidentally
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 97 at r1 (raw file):
* The returned revision will not be affected by subsequent writes to the working directory. * * @return accessor to access the current live branch
copy-paste error ;)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/AbstractFileAccessor.java, line 147 at r1 (raw file):
} public boolean agentMappingsExist() {
please add a short doc, so at least all public methods contains one
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/CachingRevisionAccess.java, line 11 at r1 (raw file):
import java.util.concurrent.ConcurrentHashMap; public class CachingRevisionAccess extends RevisionAccess {
Can you add a small doc please
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/CachingRevisionAccess.java, line 16 at r1 (raw file):
* Maps file paths to their cached contents */ private ConcurrentHashMap<String, byte[]> fileContentsCache = new ConcurrentHashMap<>();
How about using guava caches in combination with a CacheLoader instead of doing the whole caching logic by yourself?
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 159 at r1 (raw file):
* * @return The files within the current tree. *
Here again, there are many formatting changes.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 280 at r1 (raw file):
PersonIdent getCurrentAuthor() { Authentication authentication = authenticationSupplier.get(); if (authentication != null) {
I wouldn't do this change but keep it as it was.
In my oppinion, letting it fail here is better because we ensure that it is always related to a user.
If we return the system user, it may happen - at least it would be possible - that a commit or promotion is done which is not releated to a user.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingManager.java, line 28 at r1 (raw file):
* The name of the agent mappings Yaml file used to read and persist mappings. */ private static final String AGENT_MAPPINGS_FILE = "agent_mappings.yaml";
Not needed anymore.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingManager.java, line 201 at r1 (raw file):
* Adds a agent mapping at the specified index. An existing mapping will be removed if it has the same name as the given one. */ private List<AgentMapping> insertAgentMapping(List<AgentMapping> currentMappings, AgentMapping agentMapping, int index) throws IOException {
Why making it more complex and adding the need for passing the list around?
Imo the code would be much easier when we keep it as it was and just calling the serializer or the setAgentMapping here to update the mappings.
Just calling a method like addAgentMapping(mapping, index) (where the currentMappings are fetched and update) is much less complex and to understand instead the need of passing a list which is modified and has to be passed to another method.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingManager.java, line 201 at r1 (raw file):
throws IOException
Can be removed
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingSerializer.java, line 66 at r1 (raw file):
* * @param agentMappings the {@link AgentMapping}s to write to file * @param mappingsFile the target file
wrong parameter description
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/model/AgentMapping.java, line 32 at r1 (raw file):
private String name; private Branch branch;
How about naming it sourceBranch? Makes it more clearly what it is
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/utils/CancelableTask.java, line 9 at r1 (raw file):
CancelableTask
Cancellable
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/utils/CancelableTask.java, line 10 at r1 (raw file):
@Slf4j public abstract class CancelableTask<R> implements Runnable {
Im not sure if you need this class at all and use the CompletableFuture instead.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/utils/CancelableTask.java, line 14 at r1 (raw file):
* Internal flag to check if cancel has been called. */ private AtomicBoolean cancelFlag = new AtomicBoolean(false);
I think there is no benefit to use this instead of a plain boolean here
There was a problem hiding this comment.
Reviewable status: 7 of 26 files reviewed, 19 unresolved discussions (waiting on @mariusoe)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentconfiguration/AgentConfigurationManager.java, line 81 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Have you changed the formatting settings or why do we have these changes (line between return and parameter), now?
When I'm formatting the project, all these changes are rolled back to the previous state.
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentconfiguration/AgentConfigurationReloadTask.java, line 57 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
fileManager.getWorkspaceRevision()Can you store this in a local variable? otherwise it may be (very unlikely) that you get a different accessor three lines below.
Good catch
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/autocomplete/util/ConfigurationFilesCache.java, line 71 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Instead of the
CancellableTaskyou could use Java'sCompletableFuturewhich could fit for your purpose:Future<Void> future = CompletableFuture.supplyAsync(()->work).thenAccept((result)->callback); future.cancel(false);
As discussed, will keep as is due to missing interrupt support
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 47 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can you add a short doc to both fields
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 55 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
accidentlyaccidentally
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/FileManager.java, line 97 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
copy-paste error ;)
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/AbstractFileAccessor.java, line 147 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
please add a short doc, so at least all public methods contains one
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/CachingRevisionAccess.java, line 11 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Can you add a small doc please
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/CachingRevisionAccess.java, line 16 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
How about using guava caches in combination with a CacheLoader instead of doing the whole caching logic by yourself?
With guava we then would need special handling for unwrapping thrown IOExceptions...
In addition we would still need the null special case handling.
Threfore I don't think that this would reduce the code or the complexity.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 159 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Here again, there are many formatting changes.
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 280 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
I wouldn't do this change but keep it as it was.
In my oppinion, letting it fail here is better because we ensure that it is always related to a user.
If we return the system user, it may happen - at least it would be possible - that a commit or promotion is done which is not releated to a user.
Then we need an additional way of explicitly saying "Do this as system user".
I needed this change because I adjusted the AgentMappingManager to create and commit a default mapping file when no mapping exists.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingManager.java, line 28 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Not needed anymore.
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingManager.java, line 201 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Why making it more complex and adding the need for passing the list around?
Imo the code would be much easier when we keep it as it was and just calling the serializer or thesetAgentMappinghere to update the mappings.
Just calling a method likeaddAgentMapping(mapping, index)(where the currentMappings are fetched and update) is much less complex and to understand instead the need of passing a list which is modified and has to be passed to another method.
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingManager.java, line 201 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
throws IOExceptionCan be removed
Now not anymore, because setAgentMappings throws.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/AgentMappingSerializer.java, line 66 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
wrong parameter description
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/mappings/model/AgentMapping.java, line 32 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
How about naming it
sourceBranch? Makes it more clearly what it is
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/utils/CancelableTask.java, line 9 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
CancelableTaskCancellable
Done.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/utils/CancelableTask.java, line 10 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Im not sure if you need this class at all and use the
CompletableFutureinstead.
As discussed, CompletableFuture has poor cancellation support for long-running tasks
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/utils/CancelableTask.java, line 14 at r1 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
I think there is no benefit to use this instead of a plain
booleanhere
Done.
…selection # Conflicts: # components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java
There was a problem hiding this comment.
Reviewed 8 of 26 files at r1, 12 of 12 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @JonasKunz)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/CachingRevisionAccess.java, line 45 at r2 (raw file):
path
This will throw a NullPointerException in case the path is null, becaue the ConcurrentHashMap doesnt support null keys.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/CachingRevisionAccess.java, line 57 at r2 (raw file):
} List<FileInfo> fileInfos = super.listFiles(path); directoriesCache.put(path, fileInfos);
Same as above.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 163 at r2 (raw file):
* @throws IOException in case the repository cannot be read */ private boolean collectFiles(TreeWalk treeWalk, List<FileInfo> resultList) throws IOException {
Isn't this already a change which is in the master?
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/versioning/VersioningManager.java, line 280 at r1 (raw file):
Previously, JonasKunz (Jonas Kunz) wrote…
Then we need an additional way of explicitly saying "Do this as system user".
I needed this change because I adjusted theAgentMappingManagerto create and commit a default mapping file when no mapping exists.
Ok, before we overcomplicate this I think it's OK and if there should be any problems at some point - which there probably never will be - we can think about it again.
There was a problem hiding this comment.
Reviewable status: 25 of 26 files reviewed, 2 unresolved discussions (waiting on @mariusoe)
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/CachingRevisionAccess.java, line 57 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Same as above.
Here it won't occur because it is within the null check branch.
components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/file/accessor/git/RevisionAccess.java, line 163 at r2 (raw file):
Previously, mariusoe (Marius Oehler) wrote…
Isn't this already a change which is in the master?
Don't know why this is shown here, had to do a emrge due to conflicts.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
Closes #806
This change is