gerrit-3.9.2 patchset-created --change regression?

[Alon Bar-Lev]

Hi,

Until now the --change was in the restapi format of:

   urlencoded(project)~urlencoded(branch)~changeid

For example:

   myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940

It looks like now it receive the numeric change id, for example:

   myProject~master~1234

Is this on purpose? as far as I understand the numeric changeid should not be used as a reference for the import/export projects and also it is not consistent with the rest of the restapi ecosystem.

Thanks,

Alon

[Alon Bar-Lev]

Oh,

It is even worse it is without the branch name as in myProject~1234

[Alon Bar-Lev]

Hi,

I fixed this on my side by not calling ChangeTriplet and formatting the change identifier manually.

I could not find the root cause of the change.

I suggest using a common class such as ChangeTriplet to format the changes back and forth so there is a single source of truth of unique id conversion.

Then migrate all the components that are creating these unique identifiers to use this common class.

The class should also be exposed to plugins as plugins may need to convert these identifiers.

Thanks,

Alon

[Sven Selberg]

$PROJECT~$CHANGE_NUM is the recommended change identifier.
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id.

It was for a long time marked as legacy/deprecated but that is no longer the case.

Since the change-number was used to create the Note DB meta-reference you can construct the database key from this change-identifier, all other identifiers require a cache- or index lookup.

By using both PROJECT and CHANGE_NUM you should not be sensitive to collisions due to imports of projects from other Gerrit instances.

 

Thanks,

Alon

[Alon Bar-Lev]

Hi Sven,

Usually in z-stream this kind of changes should be avoided, if an application used a specific format for hooks it should continue to do so until the next y-stream and with a clear note.

What is the proper method for a plugin to be synchronized with gerrit components that produce change id? Until now I thought that `ChangeTriplet` is the class so previously we recently added `ChangeTriple.format()`, I am unsure this class is capable of parsing the recommended format... Is there a class that produces canonical change id string and can parse all supported change id notations?

Thanks,

Alon

[Sven Selberg]

On Monday, March 11, 2024 at 8:39:38 AM UTC+1 Alon Bar-Lev wrote:

On Mon, Mar 11, 2024 at 9:09 AM Sven Selberg <sven.s...@axis.com> wrote:

On Sunday, March 10, 2024 at 1:55:42 PM UTC+1 Alon Bar-Lev wrote:

$PROJECT~$CHANGE_NUM is the recommended change identifier.
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id.

It was for a long time marked as legacy/deprecated but that is no longer the case.

Since the change-number was used to create the Note DB meta-reference you can construct the database key from this change-identifier, all other identifiers require a cache- or index lookup.

By using both PROJECT and CHANGE_NUM you should not be sensitive to collisions due to imports of projects from other Gerrit instances

Hi Sven,

Usually in z-stream this kind of changes should be avoided, if an application used a specific format for hooks it should continue to do so until the next y-stream and with a clear note.

Nothing (very little) has changed in 3.9 w.r.t. the effect of using different change identifiers.
The $PROJECT~$BRANCH~$CHANGE_ID identifier had no performance implications (that I am aware of) up until the switch from RDBMS to Note DB (2.16-3.0 depending on when you migrated).
But once Note DB was used the performance/consistency implications of using other change-identifiers took effect immediately.

It is only quite recently that we realized that these limitations existed, documented them and started to promote the $PROJECT~$CHANGE_NUM as the change-identifier to use when possible.

I consider it more of a bug-fix and not a reason for another major release.

 

What is the proper method for a plugin to be synchronized with gerrit components that produce change id? Until now I thought that `ChangeTriplet` is the class so previously we recently added `ChangeTriple.format()`, I am unsure this class is capable of parsing the recommended format... Is there a class that produces canonical change id string and can parse all supported change id notations?

Personally I would NOT use ChangeTriplet given the limitations I outlined previously. That being said I wasn't even aware of the ChangeTriplet class until just now.

 

Thanks,

Alon

[Nasser Grainawi]

On Mon, Mar 11, 2024 at 2:07 AM Sven Selberg <sven.s...@axis.com> wrote:

On Monday, March 11, 2024 at 8:39:38 AM UTC+1 Alon Bar-Lev wrote:

On Mon, Mar 11, 2024 at 9:09 AM Sven Selberg <sven.s...@axis.com> wrote:

On Sunday, March 10, 2024 at 1:55:42 PM UTC+1 Alon Bar-Lev wrote:

$PROJECT~$CHANGE_NUM is the recommended change identifier.
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id.

It was for a long time marked as legacy/deprecated but that is no longer the case.

Since the change-number was used to create the Note DB meta-reference you can construct the database key from this change-identifier, all other identifiers require a cache- or index lookup.

By using both PROJECT and CHANGE_NUM you should not be sensitive to collisions due to imports of projects from other Gerrit instances

Hi Sven,

Usually in z-stream this kind of changes should be avoided, if an application used a specific format for hooks it should continue to do so until the next y-stream and with a clear note.

Nothing (very little) has changed in 3.9 w.r.t. the effect of using different change identifiers.

I think what Alon is saying is that the format of the argument passed to hooks changed. I agree that we shouldn't have that kind of API break in a stable release. @Sven Selberg do you know what change introduced that?

 

The $PROJECT~$BRANCH~$CHANGE_ID identifier had no performance implications (that I am aware of) up until the switch from RDBMS to Note DB (2.16-3.0 depending on when you migrated).
But once Note DB was used the performance/consistency implications of using other change-identifiers took effect immediately.

It is only quite recently that we realized that these limitations existed, documented them and started to promote the $PROJECT~$CHANGE_NUM as the change-identifier to use when possible.

I consider it more of a bug-fix and not a reason for another major release.

 

What is the proper method for a plugin to be synchronized with gerrit components that produce change id? Until now I thought that `ChangeTriplet` is the class so previously we recently added `ChangeTriple.format()`, I am unsure this class is capable of parsing the recommended format... Is there a class that produces canonical change id string and can parse all supported change id notations?

Personally I would NOT use ChangeTriplet given the limitations I outlined previously. That being said I wasn't even aware of the ChangeTriplet class until just now.

 

Thanks,

Alon

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/4f8b2cbc-c744-44ba-a8b0-87d3a5dda0b1n%40googlegroups.com.

[Luca Milanesio]

Hi Alon,

On 10 Mar 2024, at 10:35, Alon Bar-Lev <alon....@gmail.com> wrote:

Hi,

Until now the --change was in the restapi format of:

   urlencoded(project)~urlencoded(branch)~changeid

Looking at [1] that should still be supported.

I tried this:

curl -v https://review.gerrithub.io/changes/GerritForge%2Fgerritforge-health-service\~master\~I289903e01c2aa197f91c947d2825e7022f13dfa0

The above returned the JSON as expected.

Can you point to the API that is misbehaving? I believe that must be just a genuine bug. Di you raise it?

For example:

   myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940

It looks like now it receive the numeric change id, for example:

   myProject~master~1234

Is this on purpose? as far as I understand the numeric changeid should not be used as a reference for the import/export projects and also it is not consistent with the rest of the restapi ecosystem.

[1] https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id

Thanks,

Alon

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/CAOazyz0T8w0Au%3D2N%3DCXs9N%2BMAnuC_FBvJyTUoCmC6FxO7wwEhQ%40mail.gmail.com.

[Luca Milanesio]

On 10 Mar 2024, at 12:55, Alon Bar-Lev <alon....@gmail.com> wrote:

Hi,

I fixed this on my side by not calling ChangeTriplet and formatting the change identifier manually.

How do you call ChangeTriplet from a REST-API?

Can you share your script and REST-API you are using?

I could not find the root cause of the change.

If you raise the bug, I’ll be happy to reproduce the issue and fix it.

I suggest using a common class such as ChangeTriplet to format the changes back and forth so there is a single source of truth of unique id conversion.

Then migrate all the components that are creating these unique identifiers to use this common class.

The class should also be exposed to plugins as plugins may need to convert these identifiers.

Not sure I understand: you developed a plugin that uses ChangeTriplet and that broke with the Gerrit upgrade?

Thanks for clarifying the use-case :-)

Luca.

Thanks,

Alon

On Sun, Mar 10, 2024 at 1:01 PM Alon Bar-Lev <alon....@gmail.com> wrote:

Oh,

It is even worse it is without the branch name as in myProject~1234

On Sun, Mar 10, 2024 at 12:35 PM Alon Bar-Lev <alon....@gmail.com> wrote:

Hi,

Until now the --change was in the restapi format of:

   urlencoded(project)~urlencoded(branch)~changeid

For example:

   myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940

It looks like now it receive the numeric change id, for example:

   myProject~master~1234

Is this on purpose? as far as I understand the numeric changeid should not be used as a reference for the import/export projects and also it is not consistent with the rest of the restapi ecosystem.

Thanks,

Alon

-- 
-- 
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/CAOazyz0yFbs8v6OXOhADte96Dv_Eb7vUmKL_B0jmjC9J_KWyYQ%40mail.gmail.com.

[Alon Bar-Lev]

On Mon, Mar 11, 2024 at 9:55 PM Luca Milanesio <luca.mi...@gmail.com> wrote:

On 10 Mar 2024, at 12:55, Alon Bar-Lev <alon....@gmail.com> wrote:

Hi,

I fixed this on my side by not calling ChangeTriplet and formatting the change identifier manually.

How do you call ChangeTriplet from a REST-API?

Can you share your script and REST-API you are using?

I could not find the root cause of the change.

If you raise the bug, I’ll be happy to reproduce the issue and fix it.

I suggest using a common class such as ChangeTriplet to format the changes back and forth so there is a single source of truth of unique id conversion.

Then migrate all the components that are creating these unique identifiers to use this common class.

The class should also be exposed to plugins as plugins may need to convert these identifiers.

Not sure I understand: you developed a plugin that uses ChangeTriplet and that broke with the Gerrit upgrade?

Thanks for clarifying the use-case :-)

Luca.

Hi Luca,

There is no bug in gerrit, it was unexpected that during z-stream the format of the change id provided by the patchset-create hook was modified.

The use case is as follows:

1. The patchset-created is called for a new patchset, this requires a unique id to be recorded with information, the only unique id I am aware of is the --change value.

2. A checks plugin is being used to retrieve the information stored by (1) based on the change id it monitors.

3. The unique id is used to callback gerrit to add comments.

For this flow to work (1) and (2) should use the same unique identifier up to now it was sync using the ChangeTriplet, the recent change modified (1) and broke the use case.

It was relatively OK (not ideal) to introduce a change that modifies both (1) and (2) losing past information fetch, at least present and future would have worked.

Not mandatory now, however, I suggest implementing a utility class similar to what ChangeTriplet was, this class will produce a unique change id and be used by all components (core and plugins), it will also be capable of accepting any supported unique change id format.

I am still unsure what the root cause was as there was no direct modification that caused this nor a class I can trace.

Thanks,

Alon

[Luca Milanesio]

On 11 Mar 2024, at 20:42, Alon Bar-Lev <alon....@gmail.com> wrote:

On Mon, Mar 11, 2024 at 9:55 PM Luca Milanesio <luca.mi...@gmail.com> wrote:

On 10 Mar 2024, at 12:55, Alon Bar-Lev <alon....@gmail.com> wrote:

Hi,

I fixed this on my side by not calling ChangeTriplet and formatting the change identifier manually.

How do you call ChangeTriplet from a REST-API?

Can you share your script and REST-API you are using?

I could not find the root cause of the change.

If you raise the bug, I’ll be happy to reproduce the issue and fix it.

I suggest using a common class such as ChangeTriplet to format the changes back and forth so there is a single source of truth of unique id conversion.

Then migrate all the components that are creating these unique identifiers to use this common class.

The class should also be exposed to plugins as plugins may need to convert these identifiers.

Not sure I understand: you developed a plugin that uses ChangeTriplet and that broke with the Gerrit upgrade?

Thanks for clarifying the use-case :-)

Luca.

Hi Luca,

There is no bug in gerrit, it was unexpected that during z-stream the format of the change id provided by the patchset-create hook was modified.

The use case is as follows:

1. The patchset-created is called for a new patchset, this requires a unique id to be recorded with information, the only unique id I am aware of is the --change value.

I checked the hooks plugin on stable-3.9 and this is the way it passes the change parameters:

@Override
public void onRevisionCreated(Event event) {
  HookArgs args = hookFactory.createArgs();

  ChangeInfo c = event.getChange();
  args.add("--change", c.id);
  args.add("--kind", String.valueOf(event.getRevision().kind));
  args.addUrl(c);
  args.add("--change-owner", c.owner);
  args.add("--project", c.project);
  args.add("--branch", c.branch);
  args.add("--topic", c.topic);
  args.add("--uploader", event.getWho());
  args.add("--commit", event.getRevision().commit.commit);
  args.add("--patchset", event.getRevision()._number);

  hook.execute(c.project, args);
}

The “--change” in the above section takes the “c.id” and not the “c.tripletId”, hence it doesn’t look like it was using the triplet there.

From what I see:

- Gerrit v3.8.x returns in the c.id a triplet

- Gerrit v3.9.x returns in the c.id the format <project>~<number>

There are no changes in stable branches, whilst I acknowledge that the format has changed from v3.8.x to v3.9.x, because of [1]: looking at the change done by Marija, it was:

a) Intentional

b) Done as part of refactoring on master

c) Documented

It is quite normal during the development of Gerrit Code Review to make refactoring and introduce changes in the payload, this seemed still compatible because the change id was still valid and recognised by Gerrit.

I have to say that in [1] Marija should have added the “Release-Notes” footer, so that I could have pick that up in the release notes, in the breaking changes section.

So, I take the blame for not documenting the breaking change :-( and thanks again for reporting it !

2. A checks plugin is being used to retrieve the information stored by (1) based on the change id it monitors.

Both triplet and <project>~<number> are valid ids, and Gerrit supports both of them.

Possibly your plugin was parsing the id and expecting a triplet, whilst after [1] that was modified to have a different format.

That should have been documented.

3. The unique id is used to callback gerrit to add comments.

Sure, and <project>~<number> should equally work as id.

For this flow to work (1) and (2) should use the same unique identifier up to now it was sync using the ChangeTriplet, the recent change modified (1) and broke the use case.

Well, <project>~<number> is unique, whilst, unfortunately, the triplet is not.

I happened to me many times to have duplicates triplets because of stale indexes.

It was relatively OK (not ideal) to introduce a change that modifies both (1) and (2) losing past information fetch, at least present and future would have worked.

Not mandatory now, however, I suggest implementing a utility class similar to what ChangeTriplet was, this class will produce a unique change id and be used by all components (core and plugins), it will also be capable of accepting any supported unique change id format.

The triplet isn’t a unique identifier, the <project>~<number> is unique instead, because it is enforced by the repository refer.

I am still unsure what the root cause was as there was no direct modification that caused this nor a class I can trace.

It wasn’t a bug in this case, but a deliberate change on master at [1].

HTH

Luca.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/371515

[Luca Milanesio]

On 11 Mar 2024, at 15:58, Nasser Grainawi <nasser....@linaro.org> wrote:

On Mon, Mar 11, 2024 at 2:07 AM Sven Selberg <sven.s...@axis.com> wrote:

On Monday, March 11, 2024 at 8:39:38 AM UTC+1 Alon Bar-Lev wrote:

On Mon, Mar 11, 2024 at 9:09 AM Sven Selberg <sven.s...@axis.com> wrote:

On Sunday, March 10, 2024 at 1:55:42 PM UTC+1 Alon Bar-Lev wrote:

$PROJECT~$CHANGE_NUM is the recommended change identifier.
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id.

It was for a long time marked as legacy/deprecated but that is no longer the case.

Since the change-number was used to create the Note DB meta-reference you can construct the database key from this change-identifier, all other identifiers require a cache- or index lookup.

By using both PROJECT and CHANGE_NUM you should not be sensitive to collisions due to imports of projects from other Gerrit instances

Hi Sven,

Usually in z-stream this kind of changes should be avoided, if an application used a specific format for hooks it should continue to do so until the next y-stream and with a clear note.

Nothing (very little) has changed in 3.9 w.r.t. the effect of using different change identifiers.

I think what Alon is saying is that the format of the argument passed to hooks changed. I agree that we shouldn't have that kind of API break in a stable release. @Sven Selberg do you know what change introduced that?

That did not happen on any stable release, but on master at:

https://gerrit-review.googlesource.com/c/gerrit/+/371515

We have at times breaking changes on master, the issue here is that the change had a “Release-Notes: skip” and therefore I missed it during the release notes phase :-(

Luca.

 

The $PROJECT~$BRANCH~$CHANGE_ID identifier had no performance implications (that I am aware of) up until the switch from RDBMS to Note DB (2.16-3.0 depending on when you migrated).
But once Note DB was used the performance/consistency implications of using other change-identifiers took effect immediately.

It is only quite recently that we realized that these limitations existed, documented them and started to promote the $PROJECT~$CHANGE_NUM as the change-identifier to use when possible.

I consider it more of a bug-fix and not a reason for another major release.

 

What is the proper method for a plugin to be synchronized with gerrit components that produce change id? Until now I thought that `ChangeTriplet` is the class so previously we recently added `ChangeTriple.format()`, I am unsure this class is capable of parsing the recommended format... Is there a class that produces canonical change id string and can parse all supported change id notations?

Personally I would NOT use ChangeTriplet given the limitations I outlined previously. That being said I wasn't even aware of the ChangeTriplet class until just now.

 

Thanks,

Alon

-- 
-- 
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/4f8b2cbc-c744-44ba-a8b0-87d3a5dda0b1n%40googlegroups.com.

-- 
-- 
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/CAF6pJ8%2BPEn3q%3DaXnw4PuqCE6fZM9oQJpAZpb%3D2%2B7rBAvyOPEAg%40mail.gmail.com.

[Alon Bar-Lev]

Nobody blames anyone... It was just a surprise.

What do you think is the best method to produce the unique change id string given I have RevisionResource?

I found this, but it is a private static function and does not seem to set the id of ChangeInfo.

RevisionResource x;

ChangeJson.createFaultyChangeInfo(x.getChangeResource().getChangeData()).get().id

2. A checks plugin is being used to retrieve the information stored by (1) based on the change id it monitors.

Both triplet and <project>~<number> are valid ids, and Gerrit supports both of them.

Possibly your plugin was parsing the id and expecting a triplet, whilst after [1] that was modified to have a different format.

That should have been documented.

Thanks for finding this! as far as I can see it should have been activated only if GERRIT_BACKEND_FEATURE_RETURN_NEW_CHANGE_INFO_ID is set, interesting.

 

3. The unique id is used to callback gerrit to add comments.

Sure, and <project>~<number> should equally work as id.

For this flow to work (1) and (2) should use the same unique identifier up to now it was sync using the ChangeTriplet, the recent change modified (1) and broke the use case.

Well, <project>~<number> is unique, whilst, unfortunately, the triplet is not.

I happened to me many times to have duplicates triplets because of stale indexes.

It was the only unique identifier the hook accepts... and it is unique regardless of the indexes, not that important now, we can remove the old format somewhere along the way.

 

It was relatively OK (not ideal) to introduce a change that modifies both (1) and (2) losing past information fetch, at least present and future would have worked.

Not mandatory now, however, I suggest implementing a utility class similar to what ChangeTriplet was, this class will produce a unique change id and be used by all components (core and plugins), it will also be capable of accepting any supported unique change id format.

The triplet isn’t a unique identifier, the <project>~<number> is unique instead, because it is enforced by the repository refer.

I am still unsure what the root cause was as there was no direct modification that caused this nor a class I can trace.

It wasn’t a bug in this case, but a deliberate change on master at [1].

HTH

Luca.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/371515

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/A7917187-62CD-488F-BE5E-920C5774BEBA%40gmail.com.