Refactor AMQP logic to better isolate rust AMQP code from uAMQP code. #6008

LarryOsterman · 2024-09-18T22:31:31Z

Most of this change is refactoring code, but there is a significant difference in this PR:

doxygen really doesn't handle conditional compilation well if there are ambiguous filenames in the repo. A large part of how this refactoring works depends on the fact that there are files with the same name in two different locations, so doxygen doesn't do well with this.

To resolve the issue, I changed doxygen to scope to just files in the "inc" subdirectory and the readme.md file.

That may result in public APIs which are not in the "inc" directory being dropped, but we shouldn't have any public APIs which aren't in the "inc" subdirectory.

LarryOsterman · 2024-09-19T16:32:28Z

Ping :).

LarryOsterman · 2024-09-20T16:00:36Z

Ping please.

ahsonkhan · 2024-09-20T16:46:10Z

Hi Larry, this is a fairly large PR (I understand a lot of it is refactoring). How would you recommend we review it? Put another way, what areas should we try to focus our attention on, which you think will be the most valuable for you?

LarryOsterman · 2024-09-20T17:22:01Z

Hi Larry, this is a fairly large PR (I understand a lot of it is refactoring). How would you recommend we review it? Put another way, what areas should we try to focus our attention on, which you think will be the most valuable for you?

You should ignore the moved files - that's about 250 or so of the files in the PR.

Concentrate on the 50 or so remaining files.

The vast majority of the changes are removing #if UAMQP conditionals from the UAMQP specific code, and removing UAMQP specific code from the rust implementation.

Note that there are some vestiges of the UAMQP stuff in the rust implementations, that's done so I have boilerplate in place to make the rust implementations of stuff that's not there yet.

sdk/core/azure-core-amqp/CMakeLists.txt

…f use

…r than attempting to run; removed some uAMQP-only features (Azure#5986) * Checkpoint of connection logic * Started implementing Rust based Connection by pulling out uAMQP artifacts * Implemented AMQP Connection in Rust; started API surface refactoring for Rust APIs; Refactored tests to remove some uAMQP only elements. * Don't leak runtime context on calls * export UUID from AMQP * clang-format

…ts pass or fail at this point

ahsonkhan

Reviewed the 112/383 files as best I could at a high level.
I believe the rest are all file renames without changes.

ahsonkhan · 2024-09-20T23:11:57Z

cmake-modules/AzureDoxygen.cmake

@@ -58,6 +58,7 @@ function(generate_documentation PROJECT_NAME PROJECT_VERSION)
        set(DOXYGEN_REPEAT_BRIEF NO)

        doxygen_add_docs(${PROJECT_NAME}-docs
+            ./inc ./README.md


Will this have any impact on the "root readmes" being published for each package? For example, if we exclude https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/storage/azure-storage-blobs/README.md does that mean, the published docs won't have:
https://azuresdkdocs.blob.core.windows.net/$web/cpp/azure-storage-blobs/12.12.0/index.html

ahsonkhan · 2024-09-20T23:13:27Z

sdk/core/azure-core-amqp/CMakeLists.txt

+  src/impl/uamqp/amqp/private/session_impl.hpp
+#  src/impl/uamqp/models/amqp_detach.cpp
+#  src/impl/uamqp/models/amqp_error.cpp
+#  src/impl/uamqp/models/amqp_header.cpp
+#  src/impl/uamqp/models/amqp_message.cpp
+#  src/impl/uamqp/models/amqp_properties.cpp
+#  src/impl/uamqp/models/amqp_transfer.cpp
+#  src/impl/uamqp/models/amqp_value.cpp


Why are some of these commented out?

ahsonkhan · 2024-09-20T23:15:58Z

sdk/core/azure-core-amqp/CMakeLists.txt

+if (USE_UAMQP)
+add_library(azure-core-amqp ${AZURE_CORE_AMQP_SOURCE} ${AZURE_CORE_AMQP_HEADER} ${AZURE_UAMQP_SOURCE} $<TARGET_OBJECTS:uamqp>)
 elseif(USE_RUST_AMQP)
-add_library(azure-core-amqp ${AZURE_CORE_AMQP_SOURCE} ${AZURE_CORE_AMQP_HEADER})
+add_library(azure-core-amqp ${AZURE_CORE_AMQP_SOURCE} ${AZURE_CORE_AMQP_HEADER} ${AZURE_RUST_AMQP_SOURCE})


Are USE_UAMQP and USE_RUST_AMQP mutually exclusive? If so, should the above set(AZURE_RUST_AMQP_SOURCE ... also be in an if/elseif rather than two independent if blocks? It might be easier to read and be consistent if we combine all the if/elseif into one block.

ahsonkhan · 2024-09-20T23:18:16Z

sdk/core/azure-core-amqp/CMakeLists.txt

 endif()

 if (USE_UAMQP)
 target_include_directories(azure-core-amqp SYSTEM PRIVATE ${UAMQP_INC_FOLDER})
+target_include_directories(azure-core-amqp PRIVATE src/impl/uamqp/amqp/private src/impl/uamqp/amqp/network ${UAMQP_INC_FOLDER})


Why are we including UAMQP_INC_FOLDER twice (in both target_include_directories calls)?

ahsonkhan · 2024-09-20T23:22:05Z

sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp

-  class RustThreadContext final {
+
+  /**
+   * @brief Represents the an implementation of the rust multithreaded runtime.


Suggested change

* @brief Represents the an implementation of the rust multithreaded runtime.

* @brief Represents an implementation of the rust multithreaded runtime.

ahsonkhan · 2024-09-20T23:39:58Z

sdk/core/azure-core-amqp/src/impl/uamqp/amqp/cancellable.cpp

@@ -3,9 +3,7 @@

 #include "azure/core/amqp/internal/cancellable.hpp"

-#if ENABLE_UAMQP


Are we able to remove this now because the entire file is conditionally compiled now?

ahsonkhan · 2024-09-20T23:42:51Z

sdk/core/azure-core-amqp/src/impl/uamqp/amqp/claim_based_security.cpp

+
+using namespace Azure::Core::Diagnostics::_internal;
+using namespace Azure::Core::Diagnostics;
+using namespace Azure::Core::Amqp::_internal;


Spot checking the difference, and it looks like this file is identical to the one:
sdk/core/azure-core-amqp/src/impl/rust_amqp/amqp/claim_based_security.cpp

Except for this line 14 is above namespace Azure { namespace Core { namespace Amqp { namespace _detail { here, and below in the rust amqp impl version.

Should they be identical?
Can you explain why we need two copies for what you are trying to achieve?

ahsonkhan · 2024-09-20T23:46:42Z

sdk/core/azure-core-amqp/test/ut/link_tests.cpp

+#if ENABLE_UAMQP
    GTEST_LOG_(INFO) << LinkState::Error << LinkState::Invalid << static_cast<LinkState>(92)
                     << LinkState::HalfAttachedAttachReceived;
+#endif


I am unable to follow/understand why we end up needing the #ifs in some of these tests, in these particular places. Can you provide some context on how you went about introducing these?

ahsonkhan · 2024-09-20T23:48:01Z

sdk/eventhubs/azure-messaging-eventhubs/src/producer_client.cpp

+      if (result)
+      {
+        throw Azure::Messaging::EventHubs::_detail::EventHubsExceptionFactory::
+            CreateEventHubsException(result);
+      }
+      return true;


Should this be the inverse? That is:
if(result) return true;

ahsonkhan · 2024-09-20T23:48:23Z

sdk/core/azure-core-amqp/test/ut/CMakeLists.txt

+endif()
+if (USE_RUST_AMQP)


LarryOsterman requested review from antkmsft, ahsonkhan, gearama and RickWinter as code owners September 18, 2024 22:31

github-actions bot added Azure.Core Event Hubs labels Sep 18, 2024

LarryOsterman force-pushed the larryo/refactor_amqp branch from 5cb5d12 to 571d9b5 Compare September 19, 2024 20:32

LarryOsterman force-pushed the feature/rust_amqp branch from f72edbe to 54da1a8 Compare September 20, 2024 17:30

RickWinter approved these changes Sep 20, 2024

View reviewed changes

sdk/core/azure-core-amqp/CMakeLists.txt Show resolved Hide resolved

antkmsft approved these changes Sep 20, 2024

View reviewed changes

LarryOsterman force-pushed the larryo/refactor_amqp branch from fc7f350 to c59a137 Compare September 20, 2024 22:33

LarryOsterman requested review from a team, danieljurek, weshaggard, benbp and schaabs as code owners September 20, 2024 22:33

LarryOsterman requested review from maorleger, christothes, billwert, KarishmaGhiya and mpodwysocki September 20, 2024 22:33

LarryOsterman force-pushed the larryo/refactor_amqp branch from 8826476 to 2790615 Compare September 20, 2024 22:46

LarryOsterman force-pushed the feature/rust_amqp branch from 54da1a8 to 296f458 Compare September 20, 2024 23:00

LarryOsterman and others added 2 commits September 20, 2024 16:02

refactor uAMQP and Rust AMQP into separate implementations for ease o…

4b6d431

…f use

LarryOsterman and others added 15 commits September 20, 2024 16:02

Cleaned up some more elements; reduced scope of doxygen significantly

8dcae5d

runtime context needs to be process global not thread global; all tes…

8f023c1

…ts pass or fail at this point

clang-format

f774b73

clang fix

de1e3d4

clang fix 2

812b679

merge failures

560d488

Another merge issue

082489e

Moved vendored uAMQP to uAMQP specific directory

7ebcd7f

Moved rust_amqp code under rust_amqp directory

f30a676

Removed uAMQP conditionals from uAMQP specific code.

7ae22cd

Removed uAMQP code from non-uAMQP implementation

2474ea9

clang-format

253559f

removed some absolute paths

ddc6c0c

Merged main into branch

adf214e

More merge issues

9f73c3e

LarryOsterman force-pushed the larryo/refactor_amqp branch from 2790615 to 9f73c3e Compare September 20, 2024 23:02

LarryOsterman merged commit ec564d5 into Azure:feature/rust_amqp Sep 20, 2024
3 of 6 checks passed

LarryOsterman deleted the larryo/refactor_amqp branch September 20, 2024 23:03

ahsonkhan reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor AMQP logic to better isolate rust AMQP code from uAMQP code. #6008

Refactor AMQP logic to better isolate rust AMQP code from uAMQP code. #6008

LarryOsterman commented Sep 18, 2024 •

edited

Loading

LarryOsterman commented Sep 19, 2024

LarryOsterman commented Sep 20, 2024

ahsonkhan commented Sep 20, 2024

LarryOsterman commented Sep 20, 2024

ahsonkhan left a comment

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

ahsonkhan Sep 20, 2024

	* @brief Represents the an implementation of the rust multithreaded runtime.
	* @brief Represents an implementation of the rust multithreaded runtime.

		@@ -3,9 +3,7 @@

		#include "azure/core/amqp/internal/cancellable.hpp"

		#if ENABLE_UAMQP

Refactor AMQP logic to better isolate rust AMQP code from uAMQP code. #6008

Refactor AMQP logic to better isolate rust AMQP code from uAMQP code. #6008

Conversation

LarryOsterman commented Sep 18, 2024 • edited Loading

LarryOsterman commented Sep 19, 2024

LarryOsterman commented Sep 20, 2024

ahsonkhan commented Sep 20, 2024

LarryOsterman commented Sep 20, 2024

ahsonkhan 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

LarryOsterman commented Sep 18, 2024 •

edited

Loading