Got "This table has been migrated to NoteDb" when using its-rtc plugin in gerrit with notedb enabled

[Makson Lee]

its-base version: master

its-rtc version: master

gerrit version: stable-2.14 with notedb enabled

the following message is found in log file, so the action of its-rtc was not triggered, right? the message says the table has been migrated to notedb, so how do we fix it?

[2017-07-18 11:21:44,474] [HTTP-78] WARN  com.google.gerrit.server.extensions.events.EventUtil : Error in listener com.google.gerrit.server.events.StreamEventsApiListener for event com.google.gerrit.server.extensions.events.RevisionCreated: This table has been migrated to NoteDb

[Makson Lee]

On Tuesday, July 18, 2017 at 11:32:00 AM UTC+8, Makson Lee wrote:

its-base version: master

its-rtc version: master

gerrit version: stable-2.14 with notedb enabled

just tested it with gerrit master branch, same issue. 

[Dave Borowitz]

I just looked through the its-rtc source, I don't think it's doing anything wrong with respect to the Gerrit API that would make it not work under NoteDb.

See below, the NoteDb problem is with the stream-events listener, nothing to do with its-rtc. If its-rtc appears to not be working, I think it's unrelated to NoteDb.

On Mon, Jul 17, 2017 at 11:32 PM, Makson Lee <cdle...@gmail.com> wrote:

its-base version: master

its-rtc version: master

gerrit version: stable-2.14 with notedb enabled

When you say enabled, which options specifically are enabled (in the [noteDb "changes"] section of etc/gerrit.config)?

 

the following message is found in log file, so the action of its-rtc was not triggered, right?the

No. All listeners are called even if one of them fails. It logs the failure and then continues with the next listener:

https://gerrit.googlesource.com/gerrit/+/9743d7d8b81e1a5a809b4157a28108178d27e331/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/RevisionCreated.java#64

 

message says the table has been migrated to notedb, so how do we fix it?

[2017-07-18 11:21:44,474] [HTTP-78] WARN  com.google.gerrit.server.extensions.events.EventUtil : Error in listener com.google.gerrit.server.events.StreamEventsApiListener for event com.google.gerrit.server.extensions.events.RevisionCreated: This table has been migrated to NoteDb

Thanks for reporting this, I will look into it.

Is there anything else in the log? Something like "Failed to dispatch event" with a stack trace?

 

--
--
To unsubscribe, email repo-discuss+unsubscribe@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+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Makson Lee]

On Tuesday, July 18, 2017 at 8:23:44 PM UTC+8, Dave Borowitz wrote:

I just looked through the its-rtc source, I don't think it's doing anything wrong with respect to the Gerrit API that would make it not work under NoteDb.

See below, the NoteDb problem is with the stream-events listener, nothing to do with its-rtc. If its-rtc appears to not be working, I think it's unrelated to NoteDb.

 

On Mon, Jul 17, 2017 at 11:32 PM, Makson Lee <cdle...@gmail.com> wrote:

its-base version: master

its-rtc version: master

gerrit version: stable-2.14 with notedb enabled

When you say enabled, which options specifically are enabled (in the [noteDb "changes"] section of etc/gerrit.config)?

[noteDb "changes"]

        write = true

        read = true

        primaryStorage = NOTE_DB

        disableReviewDb = false

 

[Dave Borowitz]

On Tue, Jul 18, 2017 at 10:19 AM, Makson Lee <cdle...@gmail.com> wrote:

On Tuesday, July 18, 2017 at 8:23:44 PM UTC+8, Dave Borowitz wrote:

I just looked through the its-rtc source, I don't think it's doing anything wrong with respect to the Gerrit API that would make it not work under NoteDb.

See below, the NoteDb problem is with the stream-events listener, nothing to do with its-rtc. If its-rtc appears to not be working, I think it's unrelated to NoteDb.

Ah, thank you for the stack trace. I just looked at the its-rtc code before, which looked fine. Now I'm looking at the its-base code and it very much does not yet support NoteDb.

I am not personally volunteering to fix everyone's plugin, but this one caller is quite simple, I will do it right now :)

 

 

On Mon, Jul 17, 2017 at 11:32 PM, Makson Lee <cdle...@gmail.com> wrote:

its-base version: master

its-rtc version: master

gerrit version: stable-2.14 with notedb enabled

When you say enabled, which options specifically are enabled (in the [noteDb "changes"] section of etc/gerrit.config)?

[noteDb "changes"]

        write = true

        read = true

        primaryStorage = NOTE_DB

        disableReviewDb = false

 

--

[Makson Lee]

On Tuesday, July 18, 2017 at 10:44:17 PM UTC+8, Dave Borowitz wrote:

Ah, thank you for the stack trace. I just looked at the its-rtc code before, which looked fine. Now I'm looking at the its-base code and it very much does not yet support NoteDb.

I am not personally volunteering to fix everyone's plugin, but this one caller is quite simple, I will do it right now :)

thanks for your kindness, so is there any guideline about which api should not be used, and which one should be used instead in order to migrate a plugin to support notedb?

[luca.mi...@gmail.com]

I am quite surprised that its-base needs adaptation, it was at the end of the day just reacting to Gerrit Events.

Possibly a design issue with the plugin, I can have a look at it ...

Luca

Sent from my iPhone

--
--
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.

[Dave Borowitz]

The short answer to how to migrate a plugin is: never ever call a method on ReviewDb directly.

The NoteDb-aware APIs are slightly more complex, e.g. for its-base you would need to replace db.patchSets().get(...) with two calls, one to ChangeNotes.Factory to get a ChangeNotes, and another to PatchSetUtil to look up the patch set ID respecting the NoteDb migration state.

I got hung up converting its-base because its small test injector isn't easy to make support these new NoteDb classes.

As a corollary: most plugins are better off using the GerritApi, which is high-level, relatively simple, and guaranteed to work with NoteDb with no changes.

That's what I'm converting its-base to now :)

On Wed, Jul 19, 2017 at 9:40 AM, <luca.mi...@gmail.com> wrote:

I am quite surprised that its-base needs adaptation, it was at the end of the day just reacting to Gerrit Events.

Possibly a design issue with the plugin, I can have a look at it ...

Luca

Sent from my iPhone

On 19 Jul 2017, at 00:56, Makson Lee <cdle...@gmail.com> wrote:

On Tuesday, July 18, 2017 at 10:44:17 PM UTC+8, Dave Borowitz wrote:

Ah, thank you for the stack trace. I just looked at the its-rtc code before, which looked fine. Now I'm looking at the its-base code and it very much does not yet support NoteDb.

I am not personally volunteering to fix everyone's plugin, but this one caller is quite simple, I will do it right now :)

thanks for your kindness, so is there any guideline about which api should not be used, and which one should be used instead in order to migrate a plugin to support notedb?

--
--

To unsubscribe, email repo-discuss+unsubscribe@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+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
--
To unsubscribe, email repo-discuss+unsubscribe@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+unsubscribe@googlegroups.com.

[luca.mi...@gmail.com]

On 19 Jul 2017, at 15:13, Dave Borowitz <dbor...@google.com> wrote:

The short answer to how to migrate a plugin is: never ever call a method on ReviewDb directly.

All the change data is in the event itself, don't remember exactly the reason why should I needed to go to ReviewDB. Either a design issue or just the properties we needed where not available in the event?

The NoteDb-aware APIs are slightly more complex, e.g. for its-base you would need to replace db.patchSets().get(...) with two calls, one to ChangeNotes.Factory to get a ChangeNotes, and another to PatchSetUtil to look up the patch set ID respecting the NoteDb migration state.

I got hung up converting its-base because its small test injector isn't easy to make support these new NoteDb classes.

As a corollary: most plugins are better off using the GerritApi, which is high-level, relatively simple, and guaranteed to work with NoteDb with no changes.

Yes, absolutely. I believe at the time its-base was written, the API did not exist or were not enough I presume.

That's what I'm converting its-base to now :)

Thanks again Dave !!

Luca

[Dave Borowitz]

On Wed, Jul 19, 2017 at 1:15 PM, <luca.mi...@gmail.com> wrote:

On 19 Jul 2017, at 15:13, Dave Borowitz <dbor...@google.com> wrote:

The short answer to how to migrate a plugin is: never ever call a method on ReviewDb directly.

All the change data is in the event itself, don't remember exactly the reason why should I needed to go to ReviewDB. Either a design issue or just the properties we needed where not available in the event?

The NoteDb-aware APIs are slightly more complex, e.g. for its-base you would need to replace db.patchSets().get(...) with two calls, one to ChangeNotes.Factory to get a ChangeNotes, and another to PatchSetUtil to look up the patch set ID respecting the NoteDb migration state.

I got hung up converting its-base because its small test injector isn't easy to make support these new NoteDb classes.

As a corollary: most plugins are better off using the GerritApi, which is high-level, relatively simple, and guaranteed to work with NoteDb with no changes.

Yes, absolutely. I believe at the time its-base was written, the API did not exist or were not enough I presume.

That's what I'm converting its-base to now :)

Thanks again Dave !!

https://gerrit-review.googlesource.com/c/114490

Tests don't pass, someone more familiar with the code should take a look.

[luca.mi...@gmail.com]

On 19 Jul 2017, at 18:18, Dave Borowitz <dbor...@google.com> wrote:

On Wed, Jul 19, 2017 at 1:15 PM, <luca.mi...@gmail.com> wrote:

On 19 Jul 2017, at 15:13, Dave Borowitz <dbor...@google.com> wrote:

The short answer to how to migrate a plugin is: never ever call a method on ReviewDb directly.

All the change data is in the event itself, don't remember exactly the reason why should I needed to go to ReviewDB. Either a design issue or just the properties we needed where not available in the event?

The NoteDb-aware APIs are slightly more complex, e.g. for its-base you would need to replace db.patchSets().get(...) with two calls, one to ChangeNotes.Factory to get a ChangeNotes, and another to PatchSetUtil to look up the patch set ID respecting the NoteDb migration state.

I got hung up converting its-base because its small test injector isn't easy to make support these new NoteDb classes.

As a corollary: most plugins are better off using the GerritApi, which is high-level, relatively simple, and guaranteed to work with NoteDb with no changes.

Yes, absolutely. I believe at the time its-base was written, the API did not exist or were not enough I presume.

That's what I'm converting its-base to now :)

Thanks again Dave !!

https://gerrit-review.googlesource.com/c/114490

Thanks

Tests don't pass, someone more familiar with the code should take a look.

I'll do it

Luca