Status Update
Comments
lu...@gmail.com <lu...@gmail.com> #2
I believe the problem is that you specify only the change number in ssh -p 29418 admin@localhost gerrit index changes <C1>
.
Have you tried the format project~changeNum
?
The
lu...@gmail.com <lu...@gmail.com> #3
This is a different problem though: the API is supposed to reindex a change without using an index lookup, otherwise you have the chicken & egg situation ("How can I find a change in the index if the purpose of the API is to create the index entry for the change?").
You can reproduce the same issue without an imported change:
- Create a Gerrit instance with project P1
- Make a backup of the change index
- Create change C1 with change number N
- Stop Gerrit
- Restore the change index from backup
- Start Gerrit
- Index C1 for project P1 via SSH command:
ssh -p 29418 admin@localhost gerrit index changes P1~N
Expected behaviour
The gerrit index changes
succeeds and the change N is indexed and searchable through the UI.
Observed behaviour
The gerrit index changes
fails with the error warning: Invalid change ID project-1~1
and the change is not reindexed.
da...@gmail.com <da...@gmail.com> #4
The gerrit index
command do not thake change-id as an argument. Instead it has sub-commands, as shown in the help output:
$ ssh -p 29418 admin@localhost gerrit index --help
gerrit index [COMMAND] [ARG ...] [--] [--help (-h)]
-- : end of options (default: false)
--help (-h) : display this help text (default: true)
Available commands of gerrit index are:
activate Activate the latest index version available
changes Index changes
changes-in-project Index changes of a project
start Start the online reindexer
See 'COMMAND --help' for more information.
So calling gerrit index $changeId
will always fail.
Looking at the code (from 3.7) of IndexChangesCommand
which is responsible for gerrit index changes
it looks like it won't use the index, but go straight to NoteDb, where we still have the unambiguity when a simple change number is used.
fa...@gmail.com <fa...@gmail.com> #5
Sorry the command in the example is not correct. It should have been gerrit index changes
as you pointed out.
However, when invoking the command,
Did you somehow manage to index a change using the index command?
da...@gmail.com <da...@gmail.com> #6
After closer inspection, we do call the index even when the useIndex
of the ChangeArgumentParser.addChange()
is set to false
.
From the ChangeArgumentParser.addChange()
, we'll call the private method called changeFromNotesFactory()
, which in turn will call CangeNotes.Factory.createUsingIndexLookup()
, which will call the index to get the change data. We need to untangle this mess :|
da...@gmail.com <da...@gmail.com> #7
sorry Ponch, I haven't seen your comment before I post mine. Good to know that we're on the same page.
Did you somehow manage to index a change using the index command?
No, I was not able to index a change with the index command. I'm investigating now usage of $project~$changeNum
tuple for reindexing.
Another option would be to add a mandatory -p $project
argument, but that would break the CLI backward compatibility.
fa...@gmail.com <fa...@gmail.com>
lu...@gmail.com <lu...@gmail.com> #8
No, I was not able to index a change with the index command. I'm investigating now usage of $project~$changeNum tuple for reindexing.
Yes, that the correct syntax for reindexing a change without using the index.
lu...@gmail.com <lu...@gmail.com> #9
@Ponch @Darek after digging to the history of the code and looking at the fix, neither the imported changes nor the indexing have anything to do with the problem.
I've amended the description of the issue and going to upload a minimalistic fix, that works :-)
lu...@gmail.com <lu...@gmail.com>
ap...@google.com <ap...@google.com> #10
Branch: stable-3.7
commit 4151e6f9efd71d9239bd2b65cac17790239b454c
Author: Luca Milanesio <luca.milanesio@gmail.com>
Date: Wed Feb 21 21:30:47 2024
Remove the useIndex option in SSH commands change id parsing
The Change I34dbf53524 introduced a specific flag for parsing
the change id in SSH commands differently for some use-cases
where there is a requirement of NOT using a Lucene index lookup.
When the flag was introduced in v2.13, Gerrit had a radically
different architecture:
- The Git repository contained only the code
- All review metadata was stored in ReviewDb
- The primary key for looking up changes was the change number
which did not require any Lucene lookups
At that time in v2.13, the introduction of the useIndex option
allowed to fix a specific SSH command for reindexing a change
in case it was missed during the change creation of patch-set
upload.
See
may end up with a stale or completely absent indexing entry.
Starting from v2.15 and then up to v3.0, the removal of
ReviewDb code-base was implemented without paying too much
attention on the consequences on the useIndex flag which was
ported "syntatically" to the newer releases but lost completely
its effectiveness. Also, the SSH commands did not have historically
enough test coverage, exposing the risks of a regression.
On the other side, the change finder is now clever enough
to use the index or the underlying repository depending on
the format of the change id, which makes a lot more sense
than a static decision based on the useIndex flag.
Also remove the unneeded reload of the change notes found and
loaded by the changeFinder, which was inadvertently added as part
of Change Idaeeb7d82.
TODO: Remove the flag completely after this change is
merged to master. The flag is kept unused in stable
versions for not invalidating the public signature
of the methods, which may be used by other parts of
Gerrit or other plugins.
Bug:
Release-Notes: Fix change id parsing in SSH commands
Change-Id: Iee2057f8978a40d37804c9198df0c1c70fdcb9eb
M java/com/google/gerrit/sshd/ChangeArgumentParser.java
Description
Step to reproduce
Example commands:
Expected result
The SSH commands can parse the
P1~N
(or other change id formats) as expected and documented, consistently with what the associated REST-API do.Observed result
The SSH commands fail to parse the change id
Background
All the parsing of the change id in the SSH commands is delegated to the same class,
ChangeArgumentParser
, responsible for decoding the change id and finding the change, using either the index or performing a read on the repository.ChangeArgumentParser
was created many years ago, at the times of Gerrit v2.13 and initially made the assumption that the change id was a number, period. When the formats of change ids evolved, the secondary index lookup was available in Lucene and, eventually, NoteDb was introduced in v2.15, the class has been evolved and also shared between the REST-API and SSH interfaces.However, because of the lack of testing on the class and the SSH commands, the commands were not properly tested for the support of all the different change ids documented and, in theory, available out of the box.
Bottom line: the SSH commands never managed to align with their REST-API counterparts and were broken for many releases, at least v2.15 onwards.
The fix is straightforward and has nothing to do with logic of the API, because it is a pure parsing problem.