Refactor AMQP logic to better isolate rust AMQP code from uAMQP code. #6008
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LarryOsterman
requested review from
antkmsft,
ahsonkhan,
gearama and
RickWinter
as code owners
|
Ping :). |
LarryOsterman
force-pushed
the
larryo/refactor_amqp
branch
from
5cb5d12
to
571d9b5
Compare
|
Ping please. |
|
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. |
LarryOsterman
force-pushed
the
feature/rust_amqp
branch
from
f72edbe
to
54da1a8
Compare
RickWinter
approved these changes
Sep 20, 2024
antkmsft
approved these changes
Sep 20, 2024
LarryOsterman
force-pushed
the
larryo/refactor_amqp
branch
from
fc7f350
to
c59a137
Compare
LarryOsterman
requested review from
a team,
danieljurek,
weshaggard,
benbp and
schaabs
as code owners
LarryOsterman
requested review from
maorleger,
christothes,
billwert,
KarishmaGhiya and
mpodwysocki
LarryOsterman
force-pushed
the
larryo/refactor_amqp
branch
from
8826476
to
2790615
Compare
LarryOsterman
force-pushed
the
feature/rust_amqp
branch
from
54da1a8
to
296f458
Compare
…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
LarryOsterman
force-pushed
the
larryo/refactor_amqp
branch
from
2790615
to
9f73c3e
Compare
ahsonkhan
reviewed
Sep 20, 2024
sdk/core/azure-core-amqp/inc/azure/core/amqp/internal/common/runtime_context.hpp
Show resolved
Hide resolved
LarryOsterman
added a commit
that referenced
this pull request
Sep 24, 2024
…#6008) * refactor uAMQP and Rust AMQP into separate implementations for ease of use * Add connection support; restructured tests to fail on RUST AMQP rather than attempting to run; removed some uAMQP-only features (#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 * Cleaned up some more elements; reduced scope of doxygen significantly * runtime context needs to be process global not thread global; all tests pass or fail at this point * Merged main into branch
LarryOsterman
added a commit
that referenced
this pull request
Sep 25, 2024
…#6008) * refactor uAMQP and Rust AMQP into separate implementations for ease of use * Add connection support; restructured tests to fail on RUST AMQP rather than attempting to run; removed some uAMQP-only features (#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 * Cleaned up some more elements; reduced scope of doxygen significantly * runtime context needs to be process global not thread global; all tests pass or fail at this point * Merged main into branch
LarryOsterman
added a commit
that referenced
this pull request
Sep 30, 2024
…#6008) * refactor uAMQP and Rust AMQP into separate implementations for ease of use * Add connection support; restructured tests to fail on RUST AMQP rather than attempting to run; removed some uAMQP-only features (#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 * Cleaned up some more elements; reduced scope of doxygen significantly * runtime context needs to be process global not thread global; all tests pass or fail at this point * Merged main into branch
LarryOsterman
added a commit
that referenced
this pull request
Sep 30, 2024
…#6008) * refactor uAMQP and Rust AMQP into separate implementations for ease of use * Add connection support; restructured tests to fail on RUST AMQP rather than attempting to run; removed some uAMQP-only features (#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 * Cleaned up some more elements; reduced scope of doxygen significantly * runtime context needs to be process global not thread global; all tests pass or fail at this point * Merged main into branch
LarryOsterman
added a commit
that referenced
this pull request
Oct 2, 2024
…#6008) * refactor uAMQP and Rust AMQP into separate implementations for ease of use * Add connection support; restructured tests to fail on RUST AMQP rather than attempting to run; removed some uAMQP-only features (#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 * Cleaned up some more elements; reduced scope of doxygen significantly * runtime context needs to be process global not thread global; all tests pass or fail at this point * Merged main into branch
LarryOsterman
added a commit
that referenced
this pull request
Oct 8, 2024
…#6008) * refactor uAMQP and Rust AMQP into separate implementations for ease of use * Add connection support; restructured tests to fail on RUST AMQP rather than attempting to run; removed some uAMQP-only features (#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 * Cleaned up some more elements; reduced scope of doxygen significantly * runtime context needs to be process global not thread global; all tests pass or fail at this point * Merged main into branch
LarryOsterman
added a commit
that referenced
this pull request
Oct 14, 2024
…#6008) * refactor uAMQP and Rust AMQP into separate implementations for ease of use * Add connection support; restructured tests to fail on RUST AMQP rather than attempting to run; removed some uAMQP-only features (#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 * Cleaned up some more elements; reduced scope of doxygen significantly * runtime context needs to be process global not thread global; all tests pass or fail at this point * Merged main into branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.