-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BlazingMQ C SDK Initial Pull Request #169
base: main
Are you sure you want to change the base?
Conversation
@Simon-Sandrew Thanks for the PR! Two quick things:
|
@hallfox Would you like to take a look at whether the basic code layout and directory structure make sense? And then I can take a more-depth look. |
Addresses #114 |
src/groups/CMakeLists.txt
Outdated
@@ -4,3 +4,4 @@ | |||
add_subdirectory(mwc) | |||
add_subdirectory(bmq) | |||
add_subdirectory(mqb) | |||
add_subdirectory(z_bmq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal standards would place this package in src/wrappers/z_bmq
.
(There's no way you could have known this nor do I recall mentioning it in our previous communication so no worries, let's just move it there before we make any bigger changes.)
src/groups/z_bmq/doc/z_bmq.txt
Outdated
@@ -0,0 +1,63 @@ | |||
bmq.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole document needs to be updated to reflect the purpose and modules available in the C wrapper.
src/groups/z_bmq/z_bmq_demo/demo.c
Outdated
@@ -0,0 +1,83 @@ | |||
#include <z_bmqa_session.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should probably live in the examples directory
src/groups/z_bmq/z_bmqa/demo.h
Outdated
@@ -0,0 +1,104 @@ | |||
#include <z_bmqa_session.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file supposed to be here?
#include <bmqa_confirmeventbuilder.h> | ||
|
||
|
||
int z_bmqa_ConfirmEventBuilder__delete(z_bmqa_ConfirmEventBuilder const** builder_obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int z_bmqa_ConfirmEventBuilder__delete(z_bmqa_ConfirmEventBuilder const** builder_obj) { | |
int z_bmqa_ConfirmEventBuilder__delete(const z_bmqa_ConfirmEventBuilder** builder_obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just part of the BDE style guide
const bmqa::ConfirmEventBuilder* builder_p = reinterpret_cast<const bmqa::ConfirmEventBuilder*>(*builder_obj); | ||
delete builder_p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we like to use these BSLS_ASSERT
macros to help check the contract of functions. In this case, we should note in the docstring for this function where undefined behavior exists, and insert these assert macros to check against it.
const bmqa::ConfirmEventBuilder* builder_p = reinterpret_cast<const bmqa::ConfirmEventBuilder*>(*builder_obj); | |
delete builder_p; | |
BSLS_ASSERT(builder_obj != NULL); | |
const bmqa::ConfirmEventBuilder* builder_p = reinterpret_cast<const bmqa::ConfirmEventBuilder*>(*builder_obj); | |
delete builder_p; |
int z_bmqa_ConfirmEventBuilder__delete(z_bmqa_ConfirmEventBuilder** builder_obj); | ||
|
||
int z_bmqa_ConfirmEventBuilder__create(z_bmqa_ConfirmEventBuilder** builder_obj); | ||
|
||
int z_bmqa_ConfirmEventBuilder__addMessageConfirmation(z_bmqa_ConfirmEventBuilder* builder_obj, | ||
const z_bmqa_Message* message); | ||
|
||
int z_bmqa_ConfirmEventBuilder__addMessageConfirmationWithCookie(z_bmqa_ConfirmEventBuilder* builder_obj, | ||
const z_bmqa_MessageConfirmationCookie* cookie); | ||
|
||
int z_bmqa_ConfirmEventBuilder__reset(z_bmqa_ConfirmEventBuilder* builder_obj); | ||
|
||
int z_bmqa_ConfirmEventBuilder__messageCount(const z_bmqa_ConfirmEventBuilder* builder_obj); | ||
|
||
int z_bmqa_ConfirmEventBuilder__blob(const z_bmqa_ConfirmEventBuilder* builder_obj, z_bmqt_Blob const** blob_obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all need docstrings
typedef struct z_bmqa_ConfirmEventBuilder z_bmqa_ConfirmEventBuilder; | ||
typedef struct z_bmqa_MessageConfirmationCookie z_bmqa_MessageConfirmationCookie; | ||
|
||
int z_bmqa_ConfirmEventBuilder__delete(z_bmqa_ConfirmEventBuilder** builder_obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily agree that doing the double pointer trick for destruction is the way we should go, let me think about it.
// TESTS | ||
// ---------------------------------------------------------------------------- | ||
|
||
static void test1_session() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to need more basic tests for each one of the APIs that we create here.
…scriptionExpression, and also finished functions in QueueOptions that use Subscription objects
Address comments from PR
* Updated toString methods, updated correlationId, and added tests for correlationId * Fixed mem leaks in producer and consumer * Moved c_wrapper tutorial * Added propertytype and move includes to be withing the extern C declaration * Fixed messageproperties.h --------- Co-authored-by: Jonathan Adotey <[email protected]> Co-authored-by: Jonathan Adotey <[email protected]>
* Add files via upload * Add files via upload * changes * Delete src/docstring_replace.py * Delete src/function_comments.py --------- Co-authored-by: Fatih Orhan <[email protected]>
* Made basic test for z_bmqa_sessionevent, not tested yet * Message Event Builder Test * Worked on QueueId test * Wrote Doxygen documentation for each function in message.h
* rewrote setProperty functions for char, short, int32, int64, string, and binary, including headers * done with some headers * did get headers for bool,char,short,int32,int64 * did headers for getPropertyAsString & Binary, then did the getProperty(Or) headers until String, still have to do binary. did implementations for all these headers in C as well * did getPropertyAsBinaryOr, assuming client provides size * learned underlying rep of bsl string - simplified these functions * messageproperties done I believe, waiting on review * changes to roperty * examples
* Add files via upload * Add files via upload * changes * Delete src/docstring_replace.py * Delete src/function_comments.py * final comments * final --------- Co-authored-by: Fatih Orhan <[email protected]>
src/applications/bmqbrkr/etc/etc
Outdated
@@ -0,0 +1 @@ | |||
/Users/aaryaman/Documents/bmq/blazingmq/src/applications/bmqbrkr/etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal paths should not be committed.
@@ -0,0 +1,63 @@ | |||
z_bmq.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description needs updating
src/applications/bmqbrkr/etc/etc
Outdated
@@ -0,0 +1 @@ | |||
/Users/aaryaman/Documents/bmq/blazingmq/src/applications/bmqbrkr/etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be removed
@@ -4,3 +4,4 @@ | |||
add_subdirectory(mwc) | |||
add_subdirectory(bmq) | |||
add_subdirectory(mqb) | |||
add_subdirectory(wrappers/z_bmq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrappers directory should be below src
# Add the libbmq group library only installing the public headers | ||
add_library(z_bmq) | ||
|
||
target_compile_definitions(z_bmq PRIVATE "MWC_INTERNAL_USAGE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target_compile_definitions(z_bmq PRIVATE "MWC_INTERNAL_USAGE") |
I don't think this library should need internal usage of mwc to work
src/groups/wrappers/z_bmq/z-bmq.pc
Outdated
@@ -0,0 +1,10 @@ | |||
prefix=/usr/local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be necessary
static void test1_session() | ||
{ | ||
// run_c_producer(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo
} | ||
|
||
int z_bmqa_Session__createAsync(z_bmqa_Session** session_obj, | ||
z_bmqa_SessionEventHandler* eventHandler, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of being passed this custom SessionEventHandler
, it would be better to do something like this:
// in z_bmqa_session.h
typedef struct z_bmqa_SessionEventHandlers {
z_bmqa_OnSessionEventCb onSessionEvent,
z_bmqa_OnMessageEventCb onMessageEvent
};
int z_bmqa_Session__createAsync(z_bmqa_Session** session,
z_bmqa_SessionEventHandlers eventHandlers,
void* context,
const z_bmqt_SessionOptions* options);
// in z_bmqa_session.cpp
int z_bmqa_Session__createAsync(z_bmqa_Session** session,
z_bmqa_SessionEventHandlers eventHandlers,
const z_bmqt_SessionOptions* options) {
// ...
eventHandler_mp = bslma::ManagedPtr<CustomEventHandler>(eventHandlers, context);
session_p = new bmqa::Session(eventHandler_mp, *options_p);
// ...
}
class CustomEventHandler: public bmqa::SessionEventHandler {
private:
z_bmqa_EventHandlers d_eventHandlers;
void* d_context;
public:
CustomEventHandler(z_bmqa_EventHandlers eventHandlers, void* context):
d_eventHandlers(eventHandlers),
d_context(context) {}
void onSessionEvent(const SessionEvent& event) BSLS_KEYWORD_OVERRIDE {
z_bmqa_SessionEvent* event_p = reinterpret_cast<...>(event);
(*d_eventHandlers.onSessionEvent)(event_p, d_context);
}
void onMessageEvent(const MessageEvent& event) BSLS_KEYWORD_OVERRIDE {
// likewise...
}
};
bmqa::QueueId* queueId_p = reinterpret_cast<bmqa::QueueId*>(queueId); | ||
bmqa::CloseQueueStatus* status_p = new bmqa::CloseQueueStatus(); | ||
|
||
// Implement timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
static void test1_session() | ||
{ | ||
// run_c_producer(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
|
||
#include <stdbool.h> | ||
|
||
struct z_bmqt_CompressionAlgorithmType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses C++ constructs, it should probably just be rexposed as an enum:
typedef enum {
z_bmqt_CompresssionAlgorithmType__e_UKNOWN = -1,
...
} z_bmqt_CompressionAlgorithmType;
Unfortunately, there's no real easy way to define this enum in terms of its C++ equivalent, as that would require accessing a C++ symbol in the header file. There are probably some tricks, but I don't think it would be worth it. What we can do is add some checks at compile time that our C header definitions match the C++ definitions with a static assert:
// in z_bmqt_compressionalgorithmtype.cpp
#include <z_bmqt_compressionalgorithmtype.h>
#include <bmqt_compressionalgorithmtype.h>
#include <bslmf_assert.h>
BSLMF_ASSERT(z_bmqt_CompressionAlgorithmType__e_UNKNOWN == bmqt::CompressionAlgorithmType::e_UNKNOWN);
...
Out of interest, is there an ETA for getting an initial C SDK merged? I’m considering experimenting with Rust bindings and a C / FFI interface would be awesome. |
Hi Marvin, thanks for the interest! The ETA to merge this PR is around a month. Also, we have been experimenting with Rust bindings over this C API as well. We can find some time to discuss our ideas. |
Excellent. Around a month sounds good to me.
Background is, I just started a multi exchange data streaming project in
Rust and, obviously, BlazingMQ would be an ideal candidate as low latency
messaging middleware.
Let’s discuss and exchange ideas. Please send me a message using the
contact form below and we find sone time.
https://emet-labs.com/contact/
…On Fri, Mar 22, 2024 at 10:36 PM Yuan Jing Vincent Yan < ***@***.***> wrote:
Out of interest, is there an ETA for getting an initial C SDK merged? I’m
considering experimenting with Rust bindings and a C / FFI interface would
be awesome.
Hi Marvin, thanks for the interest! The ETA to merge this PR is around a
month. Also, we have been experimenting with Rust bindings over this C API
as well. We can find some time to discuss our ideas.
—
Reply to this email directly, view it on GitHub
<#169 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFYR7XHNUQLY2T45IDU5HRDYZQ6X7AVCNFSM6AAAAABANEW2DKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJVGI2DAOJZGA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed 23 out of 77 files (alphabetically from .gitignore
to z_bmqa_message.h
). In addition to the comments, please fix the build and formatting errors in the CI.
@@ -0,0 +1,63 @@ | |||
z_bmq.txt | |||
|
|||
@PURPOSE: Public SDK API for the BlazingMQ framework. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PURPOSE: Public SDK API for the BlazingMQ framework. | |
@PURPOSE: Public C SDK API for the BlazingMQ framework. |
|
||
@MNEMONIC: BlazingMQ (bmq) | ||
|
||
@DESCRIPTION: BlazingmQ (package group 'bmq') is a message-queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bmq
-> z_bmq
The 'bmqa' and 'bmqt' packages contain all components that constitute the | ||
public API for BlazingmQ users to use. A client should only use the | ||
components in these packages, and should not use any other package under the | ||
'bmq' package group since they are implementation components that may change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bmq
-> z_bmq
@DESCRIPTION: BlazingmQ (package group 'bmq') is a message-queue | ||
framework allowing application developers to use reliable distributed queues. | ||
|
||
The 'bmqa' and 'bmqt' packages contain all components that constitute the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bmqa
-> z_bmqa
bmqt
-> z_bmqt
'bmq' package group since they are implementation components that may change | ||
at any time. | ||
|
||
/Hierarchical Synopsis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the rest of the document to reflect z_bmq
accurately as well
|
||
typedef struct z_bmqa_Event z_bmqa_Event; | ||
|
||
int z_bmqa_Event__create(z_bmqa_Event** event_obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unimplemented and undocumented function...
|
||
int z_bmqa_Event__create(z_bmqa_Event** event_obj); | ||
|
||
int z_bmqa_Event__SessionEvent(z_bmqa_Event* event_obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unimplemented and undocumented function...
bmqa::Message* other_p = new bmqa::Message(); | ||
*other_p = message_p->clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message_p->clone()
on the next line already returns a bmqa::Message
. There is no need to call new bmqa::Message();
here. Can simplify as:
bmqa::Message* other_p = new bmqa::Message(); | |
*other_p = message_p->clone(); | |
bmqa::Message* other_p = message_p->clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, bmqa::Message::clone()
is allocator-aware. Not sure if we should worry about allocator support here though.
bsl::string data_str = ss.str(); | ||
*buffer = new char[data_str.length() + 1]; | ||
(*buffer)[data_str.length()] = '\0'; | ||
strcpy(*buffer, data_str.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the transformation via HexDumper is technically correct, it's got a few problems:
- I'm pretty sure there is an unnecessary copy with the
stringstream::str
call strcpy
is an unsafe operation and should be avoided if possible- The buffer should probably be zero initialized
My preference would be to use bdlbb::BlobUtil::copy(char *dstBuffer, const Blob& srcBlob, int position, int length)
.
Please add a unit test to ensure that z_bmqa_Message__getData()
works as intended.
return message_p->ackStatus(); | ||
} | ||
|
||
// Add once we figure out how to handle Blobs in C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this comment section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't gone through all files yet. But here are more comments.
As a general note, we should have unit tests for all non-trivial and error-prone functions.
#include <bmqa_messageevent.h> | ||
#include <bsl_sstream.h> | ||
#include <bsl_string.h> | ||
#include <z_bmqa_messageevent.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to first line of include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this means move #include <z_bmqa_messageevent.h>
but not sure
bsl::string out_str = ss.str(); | ||
|
||
*out = new char[out_str.length() + 1]; | ||
strcpy(*out, out_str.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strcpy
is an unsafe operation and should be avoided if possible. strncpy
is a safer alternative which specifies the number of bytes to be copied.
Also, please add a unit test for this toString() method.
bmqa::MessageProperties* properties_p = | ||
reinterpret_cast<bmqa::MessageProperties*>(properties_obj); | ||
|
||
//can I just use Enum like this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please finish implementation
return properties_p->setPropertyAsInt64(name_str,value); | ||
} | ||
|
||
//needs rigorous testing to see if value conversion to bsl::string is needed, or we use the char* value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is you do need a conversion to bsl::string
. But adding a test is always a good idea!
return properties_p->getPropertyAsInt64(name_str); | ||
} | ||
|
||
//docstring needs to include freeing cStr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value is a reference to propertyAsBSLString
, which is a reference to the MessageProperties
. Therefore, you should not free the returning c_str directly.
bool z_bmqa_MessageProperties__remove( | ||
z_bmqa_MessageProperties* properties_obj, | ||
const char* name, | ||
Enum *buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unconfident that passing Enum *buffer
would work properly. Could you please add a test case for this?
// MAIN PROGRAM | ||
// ---------------------------------------------------------------------------- | ||
|
||
int main(int argc, char* argv[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Very nice tests haha
mwctst::TestHelper::printTestName("BREATHING TEST"); | ||
|
||
z_bmqa_SessionEvent * obj; | ||
z_bmqa_SessionEvent__create(&obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you tested creation, you might as well test z_bmqa_SessionEvent__delete
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the remaining files. Have more comments
return 0; | ||
} | ||
|
||
// int z_bmqa_Session__confirmMessage(z_bmqa_Session* session_obj, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be implemented
// reinterpret_cast<bmqa::Session*>(session_obj); | ||
// } | ||
|
||
// int z_bmqa_Session__confirmMessageWithCookie( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be implemented
return 0; | ||
} | ||
|
||
// int z_bmqa_Session__getQueueIdWithUri(z_bmqa_Session* session_obj, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be implemented
Co-authored-by: Yuan Jing Vincent Yan <[email protected]> Signed-off-by: Simon Sandrew <[email protected]>
@pniedzielski Adding you as a reviewer |
* Add files via upload * Add files via upload * changes * Delete src/docstring_replace.py * Delete src/function_comments.py * final comments * final * deleted external files * towards future commits * pr feedbakc --------- Co-authored-by: Fatih Orhan <[email protected]>
Initial PR for the C SDK.
Requesting review on everything in the z_bmq folder.