[gRNAet7B] Added trigger show procedure #3335
Conversation
vga91
force-pushed
the
add_trigger_show_procedure
branch
from
03d7bae to
1dbba23
Compare
There was a problem hiding this comment.
I have doubts on whether the purpose of trigger.show should be to indicate whether a trigger has been discovered by the database yet or rather just list the ones the user installed (doesn't mean they've been picked up yet, but that's the spirit of eventually consistent things).
In the first case, it cannot be solved gracefully and I'd rather not merge this PR and make the user depend then on apoc.trigger.list against a user database. A trigger can be listed there but doesn't mean for another instance is discovered yet, just for the particular instance trigger.show ends up being executed on.
If we add a trigger.show, I'd rather just list the triggers that the system database is aware of (just go to the system database and grab the triggers that are there).
vga91
force-pushed
the
add_trigger_show_procedure
branch
from
c64e9ed to
4904b28
Compare
I changed the |
vga91
changed the title
There was a problem hiding this comment.
Hi Giuseppe, I'm sorry but I'm finding this PR a bit hard to review because it contains two different pieces of work:
- the changes to introduce apoc.trigger.show
- the changes to de-flake the trigger procedures
Would it be possible to split this PR into two PRs, one for each unit of work? I feel like this is generally quite a good practice to follow and can save us all some time in the long run.
core/src/main/java/apoc/trigger/TriggerHandlerNewProcedures.java
Outdated
Show resolved
Hide resolved
vga91
force-pushed
the
add_trigger_show_procedure
branch
from
8e0b2ea to
7d36b6c
Compare
vga91
changed the title
core/src/main/java/apoc/trigger/TriggerHandlerNewProcedures.java
Outdated
Show resolved
Hide resolved
|
This is great progress 😄 You have two small nitpick comments about the logic. I think that if you're able to add some docs to this PR then we can get those reviewed and merged. Thank you for all the hard work so far! |
vga91
force-pushed
the
add_trigger_show_procedure
branch
from
7d36b6c to
9d8ea45
Compare
vga91
force-pushed
the
add_trigger_show_procedure
branch
from
911633f to
05ad796
Compare
vga91
force-pushed
the
add_trigger_show_procedure
branch
from
05ad796 to
475b409
Compare
|
|
||
| `CALL apoc.trigger.show(databaseName) | it lists all installed triggers` | ||
| ¦label:procedure[] | ||
| ¦label:apoc-core[] |
There was a problem hiding this comment.
The slight issue I see is that both "apoc.trigger.show" and "apoc.trigger.list" have the same short description "it lists all installed triggers"
I would like to propose
"CALL apoc.trigger.show(databaseName) | it lists all eventually installed triggers for a database"
"CALL apoc.trigger.list() | list all currently installed triggers for all databases for the session database"
Anything you'd like to suggest is of course welcome!
| [WARNING] | ||
| ==== | ||
| Please note that, since the trigger operations are eventually consistent (based on `apoc.trigger.refresh` configuration), | ||
| the `apoc.trigger.show` may return some triggers not yet added/updated/removed. |
There was a problem hiding this comment.
"actually working triggers" vs "currently working triggers"
I don't mind which word we use in the descriptions, but if possible let's keep using the same one
There was a problem hiding this comment.
Ok, I'll use "currently working" :)
| the `apoc.trigger.show` may return some triggers not yet added/updated/removed. | ||
|
|
||
| If you want the list of actually working triggers, | ||
| you can use the xref::overview/apoc.trigger/apoc.trigger.list.adoc[apoc.trigger.list] against the user database ("neo4j" in the above case) |
There was a problem hiding this comment.
| you can use the xref::overview/apoc.trigger/apoc.trigger.list.adoc[apoc.trigger.list] against the user database ("neo4j" in the above case) | |
| you can use the xref::overview/apoc.trigger/apoc.trigger.list.adoc[apoc.trigger.list] against the session database ("neo4j" in the above case) |
…cedures#3335) * Fix flaky trigger dbms availability tests * added apoc.trigger.show proc * retry to fix flaky test by separating setLastUpdate() * removed updated output - some code refactoring * split pr - changes review * removed inner transaction * updated documentation * reset wrong generated docs * Added admin in trigger.show * small adoc changes
…cedures#3335) * Fix flaky trigger dbms availability tests * added apoc.trigger.show proc * retry to fix flaky test by separating setLastUpdate() * removed updated output - some code refactoring * split pr - changes review * removed inner transaction * updated documentation * reset wrong generated docs * Added admin in trigger.show * small adoc changes
…cedures#3335) * Fix flaky trigger dbms availability tests * added apoc.trigger.show proc * retry to fix flaky test by separating setLastUpdate() * removed updated output - some code refactoring * split pr - changes review * removed inner transaction * updated documentation * reset wrong generated docs * Added admin in trigger.show * small adoc changes
…cedures#3335) * Fix flaky trigger dbms availability tests * added apoc.trigger.show proc * retry to fix flaky test by separating setLastUpdate() * removed updated output - some code refactoring * split pr - changes review * removed inner transaction * updated documentation * reset wrong generated docs * Added admin in trigger.show * small adoc changes
* [gRNAet7B] Fixes trigger procedures in clusters (#3183) * [gRNAet7B] Leaves it working for add trigger * [gRNAet7B] Fixes trigger procedures with split LifeCycle and System utils * Deprecated old triggers and included warning - Added check to fail if they are executed against a system = FOLLOWER * Added cluster tests * removed unused import * changed msg wording * split deprecated and new triggers * Fixed TriggerExtendedTest * update apoc.systemdb.export.metadata * remove lastUpdate, rename classes * added eventual consistency * commonize trigger new and deprec. cache * ignored tests * added eventually separated tests - restored util.validateQuery - small changes * renamed Trigger classes * var names - test changes and additions * nitpick and lightened test * test flaky cluster Co-authored-by: Giuseppe Villani <[email protected]> * [gRNAet7B] Trigger procedures neo4j 5.x changes * [gRNAet7B] code clean - removed unused import * [gRNAet7B] Try fix flaky TriggerClusterRoutingTest (neo4j-contrib/neo4j-apoc-procedures#3323) * [gRNAet7B] fix some tests * [gRNAet7B] fix TriggerClusterRoutingTest * [gRNAet7B] fix TriggerRouting error message * [gRNAet7B] Added trigger show procedure (neo4j-contrib/neo4j-apoc-procedures#3335) * Fix flaky trigger dbms availability tests * added apoc.trigger.show proc * retry to fix flaky test by separating setLastUpdate() * removed updated output - some code refactoring * split pr - changes review * removed inner transaction * updated documentation * reset wrong generated docs * Added admin in trigger.show * small adoc changes * [gRNAet7B] Try de-flaky some tests with new procedure triggers (neo4j-contrib/neo4j-apoc-procedures#3355) * Try de-flaky some tests with new procedure triggers * moved TriggerTestUtil and increased trigger.refresh value * added waitDbsAvailable util * [gRNAet7B] removed unused import * [gRNAet7B] fix cluster test * [gRNAet7B] added documentation about new trigger procedures and deprecated ones (neo4j-contrib/neo4j-apoc-procedures#3325) * [gRNAet7B] added trigger new procs documentation * changes review - procedure description * changed listener wording * added system db note * var. small changes * [gRNAet7B] Fixes triggers (neo4j-contrib/neo4j-apoc-procedures#3369) * [coZSswV2] Fix typo in triggers error message * [coZSswV2] Assert new triggers can't add TransactionEventListeners to "system" * [coZSswV2] Assert old triggers can't add TransactionEventListeners to "system" * [gRNAet7B] removed unused import * [gRNAet7B] moved TriggerInfo class, removed wrong extended file * [gRNAet7B] Add @Admin to trigger procedures (neo4j-contrib/neo4j-apoc-procedures#3357) * [qwJFy8pp] Add @Admin to trigger procedures * added admin in trigger.list and docs note * changed admin doc * [gRNAet7B] moved classes - changed TriggerInfo - err. handling changes * [gRNAet7B] Cleanup trigger docs (neo4j-contrib/neo4j-apoc-procedures#3378) * [gRNAet7B] added TriggerEnterpriseFeaturesTest, removed log and session db check * [gRNAet7B] changed descriptions consistently with to Trigger.java 5.x ones Co-authored-by: Nacho Cordón <[email protected]> Co-authored-by: Daniel Leaver <[email protected]>
I guess we can't implement
apoc.trigger.show()in the same way asapoc.trigger.list,because the latter uses a
triggerHandlerfound only in system databases.So I guess we should fetch information from system nodes directly.
My doubt is that, in this way, since the triggers are eventually consistent,we could have an
apoc.trigger.showthat shows a trigger which is not actually active yet.One way to fix it would be to do
ApocConfig.apocConfig().getDatabase(databaseName).executeTransactionally(apoc.trigger.list()),but in 5.x it wouldn't work for this reason.
Or, as done in this pr, I think we should add an
updatedproperty that compares the current timestamp with that oflastUpdatedand theapoc.trigger.refreshconfiguration (so it's true iflastUpdated - currentType > interval)Obviously it is not consistent, but it's useful to give the administrator a rough idea about the trigger.
Wdyt?