Ordering of patches with cycles during upload #2524

aditya-07 · 2024-04-23T06:56:57Z

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Description

Find subgraphs (weakly connected) of Resources with cycles in them .
Pack the subgraphs in Bundles such that a single subgraph is not split between two Bundles (as it may cause server error).
Order the remaining resources and add them to the Bundle (in remaining space of or new Bundles) .

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: Bug fix

Screenshots (if applicable)

Checklist

I have read and acknowledged the Code of conduct.
I have read the Contributing page.
I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

MJ1998 · 2024-05-01T16:11:48Z

Is this ready for review ?

MJ1998

What if we use Strongly Connected Components (SCC) ?

Identify the SCCs
Consolidate all nodes in each SCC into a supernode.
Then apply the same topological sorting

So for following graph:-
1 -> 2, 3
2 -> 4, 5
4 -> 1
5 -> 6
7 -> 8, 9
8 -> 7

SCC = [{1,2,4}, {7, 8}]
We can have the topological sort as - [{3}, {6}, {5}, {1, 2, 4}, {9}, {7, 8}]

Current solution is also right but the above solution has more improvements:-

Time complexity is one-fourth (from first look).
In current approach the weakly-connected-components will go in one bundle if there is a cycle. Whereas in the above approach only the SCC will go in one bundle -> Reduces the size of the bundle a lot.

MJ1998 · 2024-05-17T06:04:33Z

It's the Tarjan's Algorithm: https://www.geeksforgeeks.org/tarjan-algorithm-find-strongly-connected-components/

engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt

jingtang10 · 2024-05-31T14:42:51Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt

@@ -67,3 +67,20 @@ internal data class PatchMapping(
  val localChanges: List<LocalChange>,
  val generatedPatch: Patch,
 )
+
+/** Structure to help describe the cyclic nature of ordered [PatchMapping]. */
+internal sealed interface OrderedMapping {


i think there is no order in this structure itself - so maybe patch mapping group is a better name - or something similar?

engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt

engine/src/main/java/com/google/android/fhir/sync/upload/request/TransactionBundleGenerator.kt

engine/src/main/java/com/google/android/fhir/sync/upload/request/UrlRequestGenerator.kt

…for Bundle generation.

jingtang10 · 2024-06-05T09:45:45Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt

+
+import kotlin.math.min
+
+internal class StronglyConnectedPatches(private val diGraph: Graph, private val nodesCount: Int) {


add comment pls

jingtang10 · 2024-06-05T09:46:06Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt

+
+import kotlin.math.min
+
+internal class StronglyConnectedPatches(private val diGraph: Graph, private val nodesCount: Int) {


Suggested change

internal class StronglyConnectedPatches(private val diGraph: Graph, private val nodesCount: Int) {

internal class StronglyConnectedPatches(private val directedGraph: Graph, private val nodeCount: Int) {

if you're having these as data members of the class - calling sscOrdered twice would not have any side effects? Just making sure... because you probably will need to make sure that nothing in your algorithm is modifying the graph and the node count.

say: the node count is the number of nodes in the directedGraph, inconsistency between the graph and the node count might generate incorrect results.

jingtang10 · 2024-06-05T10:11:10Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt

+  /**
+   * Converts a graph that has ssc to a graph with super nodes as per the provided [nodeToSuperNode]
+   * mapping.
+   *
+   * **Input**
+   * * oldGraph `[A : B, C], [B : A, C], [C : A, B, F], [D : E], [E : D], [F : G]`
+   * * nodeToSuperNode `[A:1, B:1, C:1, D:2, E:2, F:3, G:4]`
+   *
+   * **Output** `[1 : 3], [2], [3: 4]`
+   */
+  private fun transformGraphWithSSCtoGraphWithSuperNodes(


what this function actually does is: generating edges in the graph G' of super nodes from the edges in the original graph G.

so if there's an edge a->b in G, then there is an edge from S(a)->S(b) in G' where S(x) is the super node of x and S(a)!=S(b).

maybe you can add this to the description as at the moment it's not as detailed.

jingtang10 · 2024-06-05T10:14:54Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt

-    return createTopologicalOrderedList(adjacencyList).mapNotNull { resourceIdToPatchMapping[it] }
+
+    return StronglyConnectedPatches(adjacencyList, resourceIdToPatchMapping.size)
+      .sscOrdered { createTopologicalOrderedList(it) }


i think you can do

.sscOrdered(::createTopologicalOrderedList)

jingtang10 · 2024-06-05T10:20:13Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt

+
+import kotlin.math.min
+
+internal class StronglyConnectedPatches(private val diGraph: Graph, private val nodesCount: Int) {


this needs some tests. especially simple cases e.g. graph of size 1, graph of size 0. graph of size 2 with or without a single link.

jingtang10 · 2024-06-05T10:30:18Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchGenerator.kt

@@ -67,3 +67,20 @@ internal data class PatchMapping(
  val localChanges: List<LocalChange>,
  val generatedPatch: Patch,
 )
+
+/** Structure to help describe the cyclic nature of ordered [PatchMapping]. */
+internal sealed interface PatchMappingGroup {


i wonder if we can define a single structure which is a collection of patch mappings that should be uploaded together. in (possibly most) some cases, they should just be individual patch mappings, but in the case of cycles, they would be bundled together as a result of your SCC calculation.

jingtang10 · 2024-06-05T10:45:34Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/PatchOrdering.kt

-private typealias Node = String
+typealias Node = String
+
+typealias Graph = Map<Node, List<Node>>


think about this alternative definition:

data class Node(private id: String, private out: List<Node>) typealias Graph = List<Node>

and potentially this may simplify your code a bit. because you can define things like

data class SuperNode(val out: List<SuperNode>, val nodes: List<Nodes>) data class Node(val id: String, val out: List<Node>, val superNode: SuperNode)

and another thing is that - if we use this, we might be able to do away with copying the strings.

jingtang10 · 2024-06-05T10:52:43Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt

+    val intToNode = mutableMapOf<Int, String>()
+    val nodeToInt = mutableMapOf<String, Int>()


add comments here.

jingtang10 · 2024-06-05T10:52:50Z

engine/src/main/java/com/google/android/fhir/sync/upload/patch/StronglyConnectedPatches.kt

+    val sccs = IntArray(graph.size)
+    val stack = ArrayDeque<Int>()
+
+    fun dfs(at: Int) {


add comment here

Ordering of patches with cyclic during upload

05873b1

aditya-07 requested review from santosh-pingle and a team as code owners April 23, 2024 06:56

aditya-07 requested a review from vorburger April 23, 2024 06:56

aditya-07 changed the title ~~Ordering of patches with cyclic during upload~~ Ordering of patches with cycles during upload Apr 23, 2024

Ignored failing test

c9e01ea

ndegwamartin mentioned this pull request Apr 23, 2024

Migrate to the latest FHIR SDK Artifacts opensrp/fhircore#3211

Merged

11 tasks

vorburger removed their request for review April 23, 2024 10:53

dubdabasoduba mentioned this pull request May 7, 2024

Bulk import JSON FHIR resources onaio/fhir-tooling#187

Merged

2 tasks

aditya-07 added 2 commits May 14, 2024 19:13

Merge branch 'master' into ak/patchOrderWithCycles

ed07d2f

Refactored and added tests

9354c15

aditya-07 requested review from jingtang10 and MJ1998 as code owners May 16, 2024 15:04

vorburger removed the request for review from a team May 16, 2024 15:05

aditya-07 added 2 commits May 16, 2024 20:35

Merge branch 'master' into ak/patchOrderWithCycles

fb10a0f

Refactored

6575fae

MJ1998 requested changes May 17, 2024

View reviewed changes

aditya-07 added 3 commits May 27, 2024 15:59

Merge branch 'master' into ak/patchOrderWithCycles

9a9f79d

Updated code to use strongly connected resources

471e5ef

Merge branch 'master' into ak/patchOrderWithCycles

d7524d6

jingtang10 requested changes May 31, 2024

View reviewed changes

aditya-07 added 2 commits June 3, 2024 19:22

Merge branch 'master' into ak/patchOrderWithCycles

e796958

Review comments: Seperated the code for finding ssc. Added some tets …

500e7e8

…for Bundle generation.

jingtang10 requested changes Jun 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ordering of patches with cycles during upload #2524

Ordering of patches with cycles during upload #2524

aditya-07 commented Apr 23, 2024

MJ1998 commented May 1, 2024

MJ1998 left a comment •

edited

MJ1998 commented May 17, 2024

jingtang10 May 31, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024

jingtang10 Jun 5, 2024


		import kotlin.math.min

		internal class StronglyConnectedPatches(private val diGraph: Graph, private val nodesCount: Int) {

		val intToNode = mutableMapOf<Int, String>()
		val nodeToInt = mutableMapOf<String, Int>()

Ordering of patches with cycles during upload #2524

Are you sure you want to change the base?

Ordering of patches with cycles during upload #2524

Conversation

aditya-07 commented Apr 23, 2024

MJ1998 commented May 1, 2024

MJ1998 left a comment • edited

Choose a reason for hiding this comment

MJ1998 commented May 17, 2024

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

MJ1998 left a comment •

edited