Diverse flow create implementation

[IlyaMoiseev]

#1231 Create diverse flow support

---

This change is 

[IlyaMoiseev]

Reviewable status: 0 of 27 files reviewed, 1 unresolved discussion (waiting on @nikitacherevko, @sergii-iakovenko, @surabujin, and @timofei-durakov)

---

services/src/northbound/src/main/java/org/openkilda/northbound/service/impl/FlowServiceImpl.java, line 189 at r1 (raw file):

        logger.info("Create flow: {}", input);

        FlowCreateRequest payload = new FlowCreateRequest(new FlowDto(input), input.getDiverseFlowId());

This is messy solution.
We have new "diverseFlowId" property in FlowPayload, as an input for diverse feature enabling. But it isn't part of system Flow model.
To pass this property into system I choose adding them to FlowActionRequest instead mapping into FlowDto.
Yours opinions?

[sergii-iakovenko]

Reviewed 21 of 27 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 9 of 9 files at r4.
Reviewable status: 25 of 32 files reviewed, 3 unresolved discussions (waiting on @IlyaMoiseev, @nikitacherevko, @nikitamarchenko, @sergii-iakovenko, @surabujin, and @timofei-durakov)

---

services/src/kilda-core/kilda-persistence-neo4j/src/main/java/org/openkilda/persistence/repositories/impl/Neo4jFlowRepository.java, line 183 at r4 (raw file):

    @Override
    public Optional<String> getOrCreateFlowGroupId(String flowId) {
        return findFlowPairById(flowId)

It makes sense to wrap find and create operations in a transaction here.

---

services/src/messaging/src/main/java/org/openkilda/messaging/payload/flow/FlowPayload.java, line 95 at r4 (raw file):

                       @JsonProperty("description") String description,
                       @JsonProperty("last-updated") String lastUpdated,
                       @JsonProperty("diverse-flowid") String diverseFlowId,

FlowPayload is used as the response payload for GET, PUT and POST operations. So why do we need to return the diverse-flowid flag there?

---

services/src/northbound/src/main/java/org/openkilda/northbound/service/impl/FlowServiceImpl.java, line 189 at r1 (raw file):

Previously, IlyaMoiseev (Ilya Moiseev) wrote…

This is messy solution.
We have new "diverseFlowId" property in FlowPayload, as an input for diverse feature enabling. But it isn't part of system Flow model.
To pass this property into system I choose adding them to FlowActionRequest instead mapping into FlowDto.
Yours opinions?

Agree, FlowDto must not contain operation details. I think the solution would look better if you created custom payloads (CreateFlowPayload, Update..., etc) by inheriting FlowPayload.

[sergii-iakovenko]

Reviewed 7 of 7 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @IlyaMoiseev, @nikitacherevko, @nikitamarchenko, @surabujin, and @timofei-durakov)

[IlyaMoiseev]

Reviewable status: 27 of 35 files reviewed, 3 unresolved discussions (waiting on @nikitacherevko, @nikitamarchenko, @sergii-iakovenko, @surabujin, and @timofei-durakov)

---

services/src/kilda-core/kilda-persistence-neo4j/src/main/java/org/openkilda/persistence/repositories/impl/Neo4jFlowRepository.java, line 183 at r4 (raw file):

Previously, sergii-iakovenko (Sergii Iakovenko) wrote…

It makes sense to wrap find and create operations in a transaction here.

Ok, moved

---

services/src/messaging/src/main/java/org/openkilda/messaging/payload/flow/FlowPayload.java, line 95 at r4 (raw file):

Previously, sergii-iakovenko (Sergii Iakovenko) wrote…

FlowPayload is used as the response payload for GET, PUT and POST operations. So why do we need to return the diverse-flowid flag there?

Moved this field into subclasses

---

services/src/northbound/src/main/java/org/openkilda/northbound/service/impl/FlowServiceImpl.java, line 189 at r1 (raw file):

Previously, sergii-iakovenko (Sergii Iakovenko) wrote…

Agree, FlowDto must not contain operation details. I think the solution would look better if you created custom payloads (CreateFlowPayload, Update..., etc) by inheriting FlowPayload.

Reasonable, moved this field into payload subclasses

[timofei-durakov]

Reviewed 5 of 9 files at r6, 10 of 10 files at r7, 5 of 5 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nikitamarchenko, @sergii-iakovenko, and @surabujin)

[Settis]

Reviewed 5 of 5 files at r9.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @IlyaMoiseev, @nikitamarchenko, @sergii-iakovenko, and @surabujin)

---

services/src/kilda-pce/src/test/java/org/openkilda/pce/impl/InMemoryPathComputerTest.java, line 494 at r9 (raw file):

                .bandwidth(10)
                .srcSwitch(Switch.builder().switchId(new SwitchId("00:0A")).build())
                .destSwitch(Switch.builder().switchId(new SwitchId("00:0D")).build())

Is it make sense to use switchRepository.findById(new SwitchId("00:0D")).get() here?
The same with the line above.

[nikitamarchenko]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @IlyaMoiseev, @nikitamarchenko, @sergii-iakovenko, and @surabujin)

[nikitamarchenko]

Reviewed 1 of 9 files at r6, 1 of 1 files at r10, 11 of 11 files at r11.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @IlyaMoiseev, @sergii-iakovenko, and @surabujin)