[DNMY] Reworking #19

ericclappier-eaton · 2022-07-13T08:12:51Z

No description provided.

MimiXav · 2022-07-18T08:19:16Z

amqp/src/AmqpClient.cpp

-    //catch (const std::exception& e) {
-    //   logWarn("m_promiseReceiver error: {}", e.what());
-    //}
+    //m_promiseReceiver.setValue();


Remove it, this comment?

I let this comment because if we wanted to restore the wait on a stop, we know that there is an issue ...

MimiXav · 2022-07-18T08:24:30Z

amqp/src/AmqpClient.cpp

 }

 bool AmqpClient::isConnected() {
-    // Wait communication was restored
+    // test if connected
    return (m_connection && m_connection.active() && connected() == ComState::Connected);
 }

 ComState AmqpClient::connected()
 {
   //logTrace("AmqpClient::connected m_communicationState={}", m_communicationState);


take into account

MimiXav · 2022-07-18T08:28:23Z

amqp/src/AmqpClient.h

@@ -20,9 +20,10 @@
 #pragma once

 #include "MsgBusAmqpUtils.h"
+#include "../../utils/public_include/fty/messagebus2/utils/MsgBusPoolWorker.hpp"


Why this and not "MsgBusPoolWorker.hpp" and in cmake add the right line?

You are right, don't understand, it doesn't compile in my side ... I will try again

MimiXav · 2022-07-18T08:33:24Z

amqp/src/MessageBusAmqp.cpp

+        return fty::unexpected(DeliveryState::Unavailable);
+    }
+
+    if (!m_clientPtr) {


This one can be factorized with isServiceAvailable(), isn't it?

It has been made yet in AmqpClient

I mean why do you test m_clientPtr after, if the test is done inside isServiceAvailable, the line 72 is redundant

You are right, I change this

MimiXav · 2022-07-18T08:35:10Z

amqp/src/MessageBusAmqp.cpp

+    logDebug("Sending message ...");
+    proton::message msgToSend = getAmqpMessage(message);
+
+    if (!m_clientPtr) {


Same like above

It has been made yet in AmqpClient

MimiXav · 2022-07-18T08:42:25Z

amqp/src/MessageBusAmqp.cpp

 }

 const std::string& MessageBusAmqp::clientName() const noexcept
 {
-    return m_busAmqp->clientName();
+    return m_clientName;


Why did you remove the MsgBusAmqp, it's was a file (encapsulation) for fty, etn to check if the message was ok, or not, if a client want to bypass some part, he can call directly MessageBusAmqp.cpp (and h)

I think that it complicate a little the code but the first reason that I wanted to have the same class for Promise in common. With this class, it would be impossible ...

ok, but you remove this functionnality

MimiXav · 2022-07-18T08:43:10Z

common/src/Promise.cpp

+bool PromiseBase<T>::isReady() {
+    bool res = false;
+    if (m_future.valid()) {
+        //auto r = m_future.wait_for(std::chrono::seconds(0));


Take into account

MimiXav · 2022-07-18T08:43:30Z

common/src/Promise.cpp

+bool PromiseBase<T>::waitFor(const int& timeout_ms) {
+    return (
+        isReady() &&
+        m_future.wait_for(std::chrono::duration<int,std::milli>(timeout_ms)) == std::future_status::ready


MimiXav · 2022-07-18T08:51:49Z

common/public_include/fty/messagebus2/Promise.h

+    void setValue(const Message& m);
+
+    MessageBus& m_messageBus; // message bus instance
+    std::string m_queue;      // queue where the reply should arrive


Address insteafof m_queue (i.e. Address m_address);

Take into account

MimiXav · 2022-07-18T08:53:04Z

common/src/Promise.cpp

+void Promise<Message>::setValue(const Message& m) {
+    if (this->isReady()) {
+        this->m_promise.set_value(m);
+        //return {};


Just to remember that it is not possible in this case

FrancoisRegisDegott-eaton · 2022-07-21T08:54:40Z

common/public_include/fty/messagebus2/Promise.h

+    void reset();
+    bool waitFor(const int& timeout_ms);
+
+public:


Taken into account

FrancoisRegisDegott-eaton · 2022-07-21T08:55:25Z

common/public_include/fty/messagebus2/Promise.h

+    //fty::Expected<void> setValue(const Message& m);
+    void setValue(const Message& m);
+
+    MessageBus& m_messageBus; // message bus instance


private members with accessors?

Taken into account

FrancoisRegisDegott-eaton · 2022-07-21T08:56:40Z

common/src/Promise.cpp

+
+template <typename T>
+PromiseBase<T>::PromiseBase() {
+    m_future = m_promise.get_future();


naming: getFuture?

FrancoisRegisDegott-eaton · 2022-07-21T08:58:32Z

common/src/Promise.cpp

+Promise<Message>::~Promise() {
+    if(!m_address.empty()) {
+        m_messageBus.unreceive(m_address);
+        m_address = "";


could be: m_address.clear()

Remove connection error callback Remove auto accept for message

Update filter

ericclappier-eaton requested review from aquette, perrettecl, MimiXav and FrancoisRegisDegott-eaton July 13, 2022 08:12

MimiXav reviewed Jul 18, 2022

View reviewed changes

FrancoisRegisDegott-eaton approved these changes Jul 21, 2022

View reviewed changes

MimiXav approved these changes Jul 21, 2022

View reviewed changes

ericclappier-eaton added 15 commits July 25, 2022 07:53

Reworking (in progress)

65859c2

Add wrapper for promise/future

fe62dfc

Fix compilation error

deac201

Clean code

3aee400

Clean code (suite)

15da1f3

Use common Promise for request

54f7f45

Fix compilation issue

4d64221

Take into account PR remarks

754a53f

Secure message bus client pointer

60c56dd

Activate tests in jenkins

f9b6173

Secure promise already satisfied

f2d28df

Secure promise already satisfied (suite)

7fbbcdd

Workaround for passing tests (TBD)

91e19b2

amqp: Reduce number of threads for test in multi synch/asynch

f0ff641

Add protected for member in Promise

ba5ae6c

ericclappier-eaton force-pushed the reworking branch from 5075766 to ba5ae6c Compare July 25, 2022 05:54

ericclappier-eaton added 7 commits July 25, 2022 08:28

Update changelog and readme

c883f78

Fix issue with close receiver

72f3b7f

Update filter

3eb279e

Change filter format

3ac87ba

Change pipe for filter

e93d443

Fix connection error

4aec842

Remove connection error callback Remove auto accept for message

Clean code

ddcfe7c

ericclappier-eaton and others added 3 commits August 16, 2022 15:13

Update comment

df9651f

Merge pull request #1 from eric3873/filter_change

8028c5f

Update filter

Move address filter function in utilities

21ac0f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNMY] Reworking #19

[DNMY] Reworking #19

ericclappier-eaton commented Jul 13, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022

MimiXav Jul 18, 2022

ericclappier-eaton Jul 18, 2022 •

edited

Loading

FrancoisRegisDegott-eaton Jul 21, 2022

ericclappier-eaton Jul 21, 2022 •

edited

Loading

FrancoisRegisDegott-eaton Jul 21, 2022

ericclappier-eaton Jul 21, 2022

FrancoisRegisDegott-eaton Jul 21, 2022

FrancoisRegisDegott-eaton Jul 21, 2022

[DNMY] Reworking #19

Are you sure you want to change the base?

[DNMY] Reworking #19

Conversation

ericclappier-eaton commented Jul 13, 2022

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

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

ericclappier-eaton Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericclappier-eaton Jul 21, 2022 • edited Loading

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

ericclappier-eaton Jul 18, 2022 •

edited

Loading

ericclappier-eaton Jul 21, 2022 •

edited

Loading