[NOID] Fixes #2283: Utilize KernelTransaction.setStatusDetails() in 4.4 #3383

vga91 · 2022-12-22T17:01:22Z

Related to #3097 (which fails because created from a fork)

Fixes #2283

Added KernelTransaction.setStatusDetails(..) every 1000 rows.

I don't added @Context public KernelTransaction kernelTx due to #2265 (comment).

I used final KernelTransaction ktx = ((InternalTransaction) tx).kernelTransaction(); instead.

conker84 · 2023-01-19T12:27:34Z

@AzuObs can you please take a look on it?

AzuObs · 2023-01-20T09:16:15Z

Hello 👋

I'm very sorry that I've not been quick to review this. It's definitely on my TO DO list. The main reason is that I've been fairly strict about prioritising security issues reported to us privately, and about the triggers work.

I will try to review this soon, but please let me know if it's urgent in any way and I will review sooner.

AzuObs

Generally speaking I think the approach should work, and should add value to the lives of our users. So, kudos on the great start 😁

As usual I cannot let you get away scot-free. I left a few small nitpicks.

AzuObs · 2023-01-31T15:17:29Z

test-utils/src/main/java/apoc/kernel/KernelTestUtils.java

+        checkStatusDetails(db, query, params, null);
+    }
+    public static void checkStatusDetails(GraphDatabaseService db, String query, Map<String, Object> params, String startQuery) {
+        String finalStartQuery = startQuery == null ? query : startQuery;


Is this meant to be String final startQuery = ...?

AzuObs · 2023-01-31T15:30:46Z

test-utils/src/main/java/apoc/kernel/KernelTestUtils.java

+        String finalStartQuery = startQuery == null ? query : startQuery;
+
+        ExecutorService executor = Executors.newSingleThreadExecutor();
+        final Future<String> future = executor.submit(() -> db.executeTransactionally(query, params, Result::resultAsString));


I think the problem you will find here is that these tests will be flaky. Because the query starts executing right away, and could complete before the assertEventually on the next line begins, which means the assertEventually waits for something which never happens.

At least I believe that's how it works, but I'd be happy to be proven wrong.

What might be worth exploring is kicking off the assertEventually before we execute the query. If you'd like to explore this, then I think it might require some further thinking because currently assertEventually is blocking whereas we'd like the query to overlap.

AzuObs · 2023-01-31T15:33:57Z

test-utils/src/main/java/apoc/kernel/KernelTestUtils.java

+        final Future<String> future = executor.submit(() -> db.executeTransactionally(query, params, Result::resultAsString));
+
+        assertEventually(() -> TestUtil.<String>singleResultFirstColumn(db,
+                "SHOW TRANSACTIONS YIELD statusDetails, currentQuery where currentQuery STARTS WITH $startQuery RETURN statusDetails", 


Suggested change

"SHOW TRANSACTIONS YIELD statusDetails, currentQuery where currentQuery STARTS WITH $startQuery RETURN statusDetails",

"SHOW TRANSACTIONS YIELD statusDetails, currentQuery WHERE currentQuery STARTS WITH $startQuery RETURN statusDetails",

AzuObs · 2023-01-31T15:45:06Z

test-utils/src/main/java/apoc/kernel/KernelTestUtils.java

+        assertEventually(() -> TestUtil.<String>singleResultFirstColumn(db,
+                "SHOW TRANSACTIONS YIELD statusDetails, currentQuery where currentQuery STARTS WITH $startQuery RETURN statusDetails", 
+                Map.of("startQuery", finalStartQuery)),
+                StringUtils::isNotEmpty, 


StringUtils::isNotEmpty only checks that there is a statusDetails which is not empty. Would it be worth adding a more meaningful check?

AzuObs · 2023-01-31T16:04:18Z

core/src/main/java/apoc/export/util/FormatUtils.java

@@ -25,6 +25,13 @@
 * @since 23.02.16
 */
 public class FormatUtils {
+
+    public static <T> String asListed(Map<String, T> map) {


Suggested change

public static <T> String asListed(Map<String, T> map) {

public static <T> String asHyphenSeparatedList(Map<String, T> map) {

"asList" would also be fine if you prefer that 👍 "asListed" is however a bit ambiguous because it can also mean "as advertised".

AzuObs · 2023-01-31T16:39:20Z

core/src/main/java/apoc/export/util/FormatUtils.java

+        return map.entrySet()
+                .stream()
+                .map(e -> "- " + e.getKey() + ": " + e.getValue())
+                .collect(Collectors.joining("\n"));


We could consider making this into a comma separated list as that is slightly more standard. Completely up to you, though.

AzuObs · 2023-01-31T16:48:55Z

core/src/main/java/apoc/export/util/ProgressReporter.java

        acceptBatch();
    }

+    public void updateStatus(boolean updateCurrent) {


Suggested change

public void updateStatus(boolean updateCurrent) {

private void updateStatus(boolean updateCurrent) {

AzuObs · 2023-01-31T16:49:43Z

core/src/main/java/apoc/export/util/ProgressReporter.java

+    public void updateStatus(boolean updateCurrent) {
+        final Map<String, Object> statusMap = JsonUtil.convertToMap(this.progressInfo);
+        if (updateCurrent) {
+            setKernelStatusMap(tx, true, statusMap);
+        }
+        setKernelStatusMap(tx, progressInfo.nodes + progressInfo.relationships + progressInfo.properties, statusMap);


I think this function is a little bit confusing, because it's not obvious at first why we have an if statement on L131 if we end up always calling setKernelStatus on L133.

I would instead recommend creating two separate functions:

updateStatus

updateStatusPeriodically

Then, from the "update" function we can call updateStatus. And from the "nextRow" function we can call updateStatusPeriodically.

AzuObs · 2023-01-31T16:51:44Z

core/src/main/java/apoc/load/Xml.java

@@ -245,6 +247,10 @@ private void handleNode(Deque<Map<String, Object>> stack, Node node, boolean sim
        }

        if (!elementMap.isEmpty()) {
+            final int counter = stack.size();
+            final Map<String, Object> statusMap = map("curr. element", counter);


Should this map also be "map(records:, counter)" for consistency? Or is an element not the same thing as a record in this context?

AzuObs · 2023-01-31T16:53:46Z

core/src/main/java/apoc/periodic/Periodic.java

@@ -103,6 +104,8 @@ public Stream<RundownResult> commit(@Name("statement") String statement, @Name(v
                    return 0L;
                }
            }), commitErrors, failedCommits, 0L);
+            setKernelStatusMap(tx, true,
+                    Map.of("successes", batches.get() - failedBatches.get(), "errors", failedBatches.get()));


Suggested change

Map.of("successes", batches.get() - failedBatches.get(), "errors", failedBatches.get()));

Map.of("committed", batches.get() - failedBatches.get(), "failed", failedBatches.get()));

This seems more in line with our existing documentation https://neo4j.com/docs/apoc/current/overview/apoc.periodic/apoc.periodic.iterate/

Same comment for the other instances of this happening in this file.

vga91 added the 4.4 label Dec 22, 2022

vga91 force-pushed the issue_2283 branch from 44ea19d to e07a456 Compare December 22, 2022 17:03

vga91 mentioned this pull request Dec 22, 2022

[NOID] Fixes #2283: Utilize KernelTransaction.setStatusDetails() in 4.4 #3097

Closed

Lojjs assigned AzuObs Jan 4, 2023

AzuObs suggested changes Jan 31, 2023

View reviewed changes

vga91 force-pushed the issue_2283 branch from 59b211a to e2fb4e6 Compare March 16, 2023 09:34

vga91 force-pushed the issue_2283 branch 2 times, most recently from dbca89d to 2630f3c Compare April 28, 2023 13:15

vga91 added 7 commits May 19, 2023 17:21

[NOID] Fixes #2283: Utilize KernelTransaction.setStatusDetails() in 4.4

00b1174

[NOID] code clean

36d02a9

[NOID] changed dbms.listTransaction with SHOW TRANSACTIONS

e650412

[NOID] changed dbms.listTransactions with SHOW TRANSACTIONS

152faba

[NOID] removed unused import

8c80e51

[NOID] changed setStatusDetails for each 1000th lines

2a4736f

[NOID] try de-flake tests

644daf3

vga91 force-pushed the issue_2283 branch from 2630f3c to 9c711fc Compare May 19, 2023 15:25

[NOID] Changes review

cc1e933

vga91 force-pushed the issue_2283 branch from 9c711fc to cc1e933 Compare June 21, 2023 08:26

AzuObs removed their assignment Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NOID] Fixes #2283: Utilize KernelTransaction.setStatusDetails() in 4.4 #3383

[NOID] Fixes #2283: Utilize KernelTransaction.setStatusDetails() in 4.4 #3383

vga91 commented Dec 22, 2022 •

edited

conker84 commented Jan 19, 2023

AzuObs commented Jan 20, 2023 •

edited

AzuObs left a comment

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

AzuObs Jan 31, 2023

	"SHOW TRANSACTIONS YIELD statusDetails, currentQuery where currentQuery STARTS WITH $startQuery RETURN statusDetails",
	"SHOW TRANSACTIONS YIELD statusDetails, currentQuery WHERE currentQuery STARTS WITH $startQuery RETURN statusDetails",

	public static <T> String asListed(Map<String, T> map) {
	public static <T> String asHyphenSeparatedList(Map<String, T> map) {

	public void updateStatus(boolean updateCurrent) {
	private void updateStatus(boolean updateCurrent) {

	Map.of("successes", batches.get() - failedBatches.get(), "errors", failedBatches.get()));
	Map.of("committed", batches.get() - failedBatches.get(), "failed", failedBatches.get()));

[NOID] Fixes #2283: Utilize KernelTransaction.setStatusDetails() in 4.4 #3383

Are you sure you want to change the base?

[NOID] Fixes #2283: Utilize KernelTransaction.setStatusDetails() in 4.4 #3383

Conversation

vga91 commented Dec 22, 2022 • edited

conker84 commented Jan 19, 2023

AzuObs commented Jan 20, 2023 • edited

AzuObs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vga91 commented Dec 22, 2022 •

edited

AzuObs commented Jan 20, 2023 •

edited