Status Update
Comments
br...@google.com <br...@google.com> #2
This is not a replication
plugin problem, but a PluginLoader
issue:
- The replication plugin exposes an
ApiModule
in master - The
ApiModule
is loaded into theapiInjector
of theServerPlugin
- All the plugins with an
ApiModule
andapiInjector
are chained together
Whilst the start()
was fully implemented, the stop()
was not, see below:
protected void stop(PluginGuiceEnvironment env) {
if (serverManager != null) {
RequestContext oldContext = env.enter(this);
try {
serverManager.stop();
} finally {
env.exit(oldContext);
}
serverManager = null;
sysInjector = null;
sshInjector = null;
httpInjector = null;
}
}
There isn't anything that manages the apiInjector
therefore the unloading keeps the injections as-is. When the plugin is reloaded, then boom the injections are duplicated.
I know how to fix it, leave it with me!
lu...@gmail.com <lu...@gmail.com> #3
@Saša see an initial fix here:
This formally works, BUT isn't ready yet because in addition to keeping the old interface definition (acceptable IMHO) it also references the old classes, making the new reloaded code unreachable.
It is late now, will resume the efforts tomorrow.
po...@gmail.com <po...@gmail.com> #4
I believe the overall issue here is that, after ApiModule
, which is the lightweight version of the libModule exposing just an API.
The ApiModule
are supposed to be loaded only once and used as base injector for all plugins. When replication became an ApiModule
has lost its ability to be dynamically reloaded.
The best solution would be to introduce a new target in the replication plugin to generate the ApiModule
as standalone jar (e.g. replication-api.jar
) and leave the replication.jar
as a regular plugin.
We can also amend the x-plugin support in future versions of Gerrit to allow reloadable ApiModule
, but that is a much more complex exercise, because it involves the construction of a full dependency graph of the dependencies and reload all the dependent plugins when an ApiModule
is reoaded.
po...@gmail.com <po...@gmail.com> #5
Switching back the component to replication
; I have a possible solution that allows:
- Keeping the ability to use x-plugin communication to extend the replication plugin with
ApiModule
- Allow reloading the
replication.jar
dynamically and provide new code - Keeping the initial replication API implementation across reloads
@Saša @Darek please have a look at
br...@google.com <br...@google.com> #6
I have also finalised the check on the x-plugin management of the PluginLoader
for preventing a plugin with ApiModule
to be stopped, unloaded, reloaded or restarted:
They should be both ready for review!
br...@google.com <br...@google.com>
ap...@google.com <ap...@google.com> #7
ap...@google.com <ap...@google.com> #8
Branch: master
commit 5e33f8344e3855478914a5c530a327f888bd8c8e
Author: Luca Milanesio <luca.milanesio@gmail.com>
Date: Sat Feb 10 21:55:12 2024
Decouple replication-api.jar from the main replication plugin
The Change Ib7a04eea5 made the whole replication plugin an ApiModule
which becomes the base injector for all plugins and forbids reloads,
(see the Iac2851022ea for enforcing it at Gerrit level)
even when there is no intention to use th extension points.
All plugins that expose an ApiModule have the following limitations:
- Cannot add extra dependencies
- Cannot be reloaded
- Are the base injectors for all plugins
The above limitations aren't acceptable for the whole replication
plugin when used standalone without extensions.
Define a new replication-api target that generates the sole ApiModule
needed for extending the replication plugin functionality.
All the classes that are part of the replication-api are moved
into a separate c.g.g.p.replication.api package so that can be
included in the replication-api.jar plugin.
The gerrit.war will continue to include only the replication.jar which
will still work out of the box. Anyone willing to customise its behaviour
through the API will have to build the replication-api target.
Also fix the MergedConfigResource that was assuming that the ApiModule
was always installed and was binding DynamicItem<ReplicationConfigOverrides>.
Bug:
Release-Notes: Decouple replication-api.jar from the main replication plugin
Change-Id: Ic432fc77daf1162368abd65f64c463e4ef4f5d6d
M BUILD
M src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecorator.java
M src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadRunnable.java
M src/main/java/com/googlesource/gerrit/plugins/replication/AutoReloadSecureCredentialsFactoryDecorator.java
M src/main/java/com/googlesource/gerrit/plugins/replication/CreateProjectTask.java
M src/main/java/com/googlesource/gerrit/plugins/replication/DestinationsCollection.java
M src/main/java/com/googlesource/gerrit/plugins/replication/FileConfigResource.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ListCommand.java
M src/main/java/com/googlesource/gerrit/plugins/replication/MergedConfigResource.java
M src/main/java/com/googlesource/gerrit/plugins/replication/OnStartStop.java
M src/main/java/com/googlesource/gerrit/plugins/replication/PushOne.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImpl.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigModule.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationDestinations.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationExtensionPointModule.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationQueue.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesUpdater.java
M src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
M src/main/java/com/googlesource/gerrit/plugins/replication/SshHelper.java
M src/main/java/com/googlesource/gerrit/plugins/replication/api/ApiModule.java
M src/main/java/com/googlesource/gerrit/plugins/replication/api/ConfigResource.java
M src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationConfig.java
M src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationConfigOverrides.java
M src/main/java/com/googlesource/gerrit/plugins/replication/api/ReplicationPushFilter.java
M src/main/resources/Documentation/extension-point.md
M src/test/java/com/googlesource/gerrit/plugins/replication/AbstractConfigTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/AutoReloadConfigDecoratorTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/FanoutConfigResourceTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/FileConfigResourceTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/MergedConfigResourceTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/PushOneTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationConfigImplTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesUpdaterTest.java
M src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationStorageDaemon.java
M src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationStorageIT.java
M src/test/java/com/googlesource/gerrit/plugins/replication/TestReplicationModule.java
Description
Step to reproduce
http://localhost:8080/c/P1/+/1/comment/f8b5cdb0_0527322b/
)Expected result
The page is correctly loaded and it is pointing to the selected comment
Observed result
The PolyGerrit URI renders a pop-up that shows the following content:
This is happening from stable-3.9.
Background
The imported changes are discoverable using any property but not its change number alone, this is by design from v3.7 onwards. The change comment has the project in the URL, so the project is accessible and the change should be fully visible.
The error
undefined~204910
on the GUI indicates that the front-end was not able to detect the project in the URL and instead triggered a backend lookup by change number, which isn't supported for imported changes. The front-end should have instead used the project name in the URL, because it was fully available from the beginning of the lifecycle of the page.The comment route in the front-end is not a
mapRouteState
, so I wonder if the project is not stored in the state and it leads to the lookup to fail:Every other part of the change is rendered as expected, so it must be something more specific to this particular use-case.