Skip to content

PROTON-2307 Allow access to connection properties in cpp binding#278

Closed
pjfawcett wants to merge 1 commit intoapache:masterfrom
pjfawcett:PROTON-2307-allow-access-to-connection-properties
Closed

PROTON-2307 Allow access to connection properties in cpp binding#278
pjfawcett wants to merge 1 commit intoapache:masterfrom
pjfawcett:PROTON-2307-allow-access-to-connection-properties

Conversation

@pjfawcett
Copy link
Copy Markdown
Contributor

Allow setting of connection properties via connection_options
Allow reading of properties on incoming connections
Add unit test to container_test.cpp

@pjfawcett
Copy link
Copy Markdown
Contributor Author

pjfawcett commented Dec 3, 2020

The AppVeyor "Visual Studio 2015" build failed due to a problem with Microsoft vcpkg. It looks like this has now been fixed
Is there a way to force a re-run of the AppVeyor build or do I need to do a "no-op push"?

@jiridanek
Copy link
Copy Markdown
Contributor

I don't see option to rerun Appveyor, so the empty commit or empty --amend is probably the only way.

@pjfawcett pjfawcett force-pushed the PROTON-2307-allow-access-to-connection-properties branch from 13caa7b to 1e7d52f Compare December 8, 2020 11:22
@pjfawcett
Copy link
Copy Markdown
Contributor Author

This is my first Qpid Pull Request hence I am unfamiliar with the process and would welcome some advice.

Firstly, what should I do about the failed builds? They seem to be related to errors/problems in the build system(s) rather than to changes I have made, but I might be missing something.

Secondly, and connected, do I need to get this PR to pass all the build checks before it is eligible for review, or is there something else I have to do (apart from being more patient) ?

Thanks

@gemmellr
Copy link
Copy Markdown
Member

I believe some of the builds are a little flakey in CI (and there are also some tests enabled in CI that aren't in the default build because of that). If you are confident the failures aren't from your changes then feel free to just say so. Another approach is to give the build a prod and see if anything changes (e.g git commit --amend --reset-author to redo the commit, followed by a force push).

You don't need to get the tests passing to elicit a review, you just need someone with the requisite knowledge to actually come along (which is not me I'm afraid).

@pjfawcett
Copy link
Copy Markdown
Contributor Author

Thanks Robbie.  I'll force another push. I was wary of doing it too repeatedly having seen some recent messages about Travis. CI limits (though later messages suggest that isn't relevant)

@pjfawcett pjfawcett force-pushed the PROTON-2307-allow-access-to-connection-properties branch from 1e7d52f to d58c0a9 Compare December 10, 2020 13:44
Copy link
Copy Markdown
Member

@astitcher astitcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjfawcett Firstly many apologies it has taken me so long to review this PR.

I really think this is a useful addition to the API.

I spent a while thinking about whether using std::map<> was a good idea in the context of this API, and I concluded that it makes sense even if it is a rather heavyweight object to be returning. Another alternative might be to just return a proton::map<> which is comparatively lightweight, but not all that convenient without first converting it to a std::map just as you did internally.

I've a few necessary changes below , but otherwise I think this is good essentially as-is.

I also note that session and link objects also have properties (but these maybe not as useful as the connection properties).

If you rebase this change on master I think you can drop the Visual Studio hacks and the tests should pass as appveyor have now fixed their VS2015 image.

@pjfawcett pjfawcett force-pushed the PROTON-2307-allow-access-to-connection-properties branch from 19d3080 to f5a911d Compare January 26, 2021 11:46
@pjfawcett
Copy link
Copy Markdown
Contributor Author

@astitcher regarding properties on session and link: I'd be happy to implement similar access to those if it was thought to be useful - probably under a follow-on ticket?

@astitcher
Copy link
Copy Markdown
Member

@astitcher regarding properties on session and link: I'd be happy to implement similar access to those if it was thought to be useful - probably under a follow-on ticket?

That would be great! I'll try to get to it much sooner next time!

@astitcher
Copy link
Copy Markdown
Member

@pjfawcett If you squash rebase these 2 commits then I'll be happy to merge. (I can do it too, but it'll be a little easier for you)

Allow access to properties on incoming connections
@pjfawcett pjfawcett force-pushed the PROTON-2307-allow-access-to-connection-properties branch from f5a911d to 55ad0e8 Compare January 26, 2021 15:43
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 26, 2021

Codecov Report

Merging #278 (f5a911d) into master (0ee9ad7) will increase coverage by 4.66%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   79.56%   84.23%   +4.66%     
==========================================
  Files         343       46     -297     
  Lines       43066     2341   -40725     
==========================================
- Hits        34267     1972   -32295     
+ Misses       8799      369    -8430     
Impacted Files Coverage Δ
ruby/lib/core/message.rb 57.05% <0.00%> (-31.77%) ⬇️
ruby/lib/util/error_handler.rb 59.37% <0.00%> (-28.13%) ⬇️
ruby/lib/codec/data.rb 76.52% <0.00%> (-19.57%) ⬇️
ruby/lib/codec/mapping.rb 88.17% <0.00%> (-1.08%) ⬇️
python/proton/_transport.py
python/proton/__init__.py
cpp/src/decimal.cpp
cpp/src/proton_bits.cpp
c/src/proactor/epoll_raw_connection.c
cpp/include/proton/null.hpp
... and 287 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ee9ad7...55ad0e8. Read the comment docs.

@pjfawcett
Copy link
Copy Markdown
Contributor Author

Squashed into single commit.
Ticket PROTON-2330 opened for sessions / links

@asfgit asfgit closed this in 61e8b0d Jan 26, 2021
@pjfawcett pjfawcett deleted the PROTON-2307-allow-access-to-connection-properties branch January 28, 2021 14:14
@pjfawcett pjfawcett restored the PROTON-2307-allow-access-to-connection-properties branch January 28, 2021 14:14
jiridanek pushed a commit to jiridanek/qpid-proton that referenced this pull request Mar 3, 2023
…tions

Allow access to properties on incoming connections

closes apache#278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants