feat: implement azure blob storage action #13069

thalesmg · 2024-05-17T20:16:40Z

Fixes https://emqx.atlassian.net/browse/EMQX-12280

Release version: e5.8

Summary

PR Checklist

Please convert it to a draft if any of the following conditions are not met. Reviewers may skip over until all the items are checked:

Added tests for the changes
Added property-based tests for code which performs user input validation
Changed lines covered in coverage report
Change log has been added to changes/(ce|ee)/(feat|perf|fix|breaking)-<PR-id>.en.md files
For internal contributor: there is a jira ticket to track this change
Created PR to emqx-docs if documentation update is required, or link to a follow-up jira ticket
Schema changes are backward compatible

Checklist for CI (.github/workflows) changes

If changed package build workflow, pass this action (manual trigger)
Change log has been added to changes/ dir for user-facing artifacts update

keynslug

🚀

keynslug · 2024-05-24T15:24:44Z

apps/emqx_bridge_azure_blob_storage/src/emqx_bridge_azure_blob_storage_action_schema.erl

+                    desc => ?DESC("aggregation_max_records")
+                }
+            )},
+        {max_block_size,


Putting it here feels odd, because AFAICS it doesn't affect the aggregation itself, only the upload process.

Indeed. Moved to action parameters. ✔️

keynslug · 2024-05-24T16:01:40Z

apps/emqx_bridge_azure_blob_storage/src/emqx_bridge_azure_blob_storage_connector.erl

+    case BufferSize > MaxBlockSize of
+        true ->
+            {IOData, NewBuffer} = take_chunk(Buffer, MaxBlockSize),
+            ?tp(azure_blob_storage_will_write_chunk, #{}),
+            do_process_write(IOData, NewBuffer, TransferState0);
+        false ->
+            NewBuffer = [],
+            do_process_write(Buffer, NewBuffer, TransferState0)
+    end.


Apparently having this append / write split backfires slightly. 🫠

Honestly both of those things feel suboptimal:

Call Azure API on every append, which is usually on the scale of 1-10s of KiBs assuming moderate event rate.

Cut >4GiB large buffer somewhere close to its end, which will likely cause 2-3x of that in heap memory consumption. This most likely will never happen, but still.

I wonder what if:

Introduce min_block_size config parameter, with something like 10 MiB as the default.

During append: append as usual, but put the buffer under some next_block map entry if its size is larger that min_block_size (i.e. minimize latency impact / overhead / cost (?) of too frequent API requests).

However, if it's to become larger than max_block_size does not append but instead put existing buffer under next_block key (i.e. avoid cutting iolists).

During write, write next_block if it's defined.

On more thing: currently, having a single append larger than 4 GiB is impossible I think, because of this. Admittedly, it's sort of an implicitly defined limit, but I'd argue still could be relied upon to get rid of complexity of "cut off exactly this much of this iolist()".

keynslug · 2024-05-24T16:06:05Z

apps/emqx_bridge_azure_blob_storage/src/emqx_bridge_azure_blob_storage_connector.erl

+lookup_buffer_var([<<"datetime">>, Format], #{since := Since}) ->
+    {ok, format_timestamp(Since, Format)};
+lookup_buffer_var([<<"datetime_until">>, Format], #{until := Until}) ->
+    {ok, format_timestamp(Until, Format)};
+lookup_buffer_var([<<"sequence">>], #{seq := Seq}) ->
+    {ok, Seq};
+lookup_buffer_var([<<"node">>], #{}) ->
+    {ok, mk_fs_safe_string(atom_to_binary(erlang:node()))};
+lookup_buffer_var(_Binding, _Context) ->
+    {error, undefined}.


Might be a good time to introduce something like a small emqx_connector_aggreg_buffer_ctx module, that implements emqx_template behaviour for #buffer{}s, and put this stuff (except for <<"node">>) there? Though we'd have to keep mk_fs_safe_string/1 on this level. Not the prettiest solution, but could help minimize code duplication and the need for that weird buffer_map() type.

Fixes emqx#13069 (comment)

keynslug

Great! A couple of nits.

apps/emqx_bridge_azure_blob_storage/src/emqx_bridge_azure_blob_storage_action_schema.erl

keynslug · 2024-05-29T08:17:27Z

apps/emqx_bridge_azure_blob_storage/src/emqx_bridge_azure_blob_storage_action_schema.erl

+    ];
+fields(common_action_parameters) ->
+    [
+        {max_block_size,


Q: Is there anything this parameter may affect for direct uploads? The idea is to prematurely drop the event on the floor if it's for some reason >4GiB in size?

Good point, currently we don't check it, but this limit indeed is Azure's maximum upload per block.
(More precisely, for a single "Put Block Blob" call, it's possible to send 5000 MiB, and each "Put Blob" is 4000 MiB)

Added a check and test here: a92eb3c

apps/emqx_connector_aggregator/src/emqx_connector_aggregator_schema.erl

zmstone · 2024-05-31T13:15:56Z

apps/emqx_connector_aggregator/src/emqx_connector_aggregator_schema.erl

+container() ->
+    {container,
+        hoconsc:mk(
+            %% TODO: Support selectors once there are more than one container.


This comment was just transported from its previous location.

i.e. also the product of a refactoring

apps/emqx_bridge_azure_blob_storage/src/emqx_bridge_azure_blob_storage_action_schema.erl

Fixes https://emqx.atlassian.net/browse/EMQX-12280

…er_ctx`

Fixes emqx#13069 (comment)

thalesmg force-pushed the azure-blob-storage-action-m-20240508 branch 16 times, most recently from f5c3cc2 to acb4c32 Compare May 22, 2024 20:57

thalesmg marked this pull request as ready for review May 23, 2024 12:45

thalesmg requested review from JimMoen and a team as code owners May 23, 2024 12:45

qzhuyan previously approved these changes May 23, 2024

View reviewed changes

thalesmg requested review from kjellwinblad and keynslug May 23, 2024 16:21

keynslug reviewed May 24, 2024

View reviewed changes

thalesmg added a commit to thalesmg/emqx that referenced this pull request May 27, 2024

fix: avoid uploading blocks too frequently and splitting large buffers

35c16ff

Fixes emqx#13069 (comment)

thalesmg dismissed qzhuyan’s stale review via 35c16ff May 27, 2024 17:59

thalesmg force-pushed the azure-blob-storage-action-m-20240508 branch from acb4c32 to 35c16ff Compare May 27, 2024 17:59

thalesmg added a commit to thalesmg/emqx that referenced this pull request May 28, 2024

fix: avoid uploading blocks too frequently and splitting large buffers

81964f1

Fixes emqx#13069 (comment)

thalesmg force-pushed the azure-blob-storage-action-m-20240508 branch 2 times, most recently from 48c4e20 to c8ae506 Compare May 28, 2024 21:29

keynslug reviewed May 29, 2024

View reviewed changes

thalesmg requested review from keynslug May 29, 2024 20:44

zmstone reviewed May 31, 2024

View reviewed changes

apps/emqx_connector_aggregator/src/emqx_connector_aggregator_schema.erl Show resolved Hide resolved

zmstone reviewed May 31, 2024

View reviewed changes

apps/emqx_bridge_azure_blob_storage/src/emqx_bridge_azure_blob_storage_action_schema.erl Show resolved Hide resolved

thalesmg added 9 commits May 31, 2024 11:07

refactor(connector aggregator): move shared schemas to app

729441d

refactor: simplify resource_opts definition

60d24c6

feat: implement azure blob storage action

c916c83

Fixes https://emqx.atlassian.net/browse/EMQX-12280

refactor: move common templating logic to `emqx_connector_aggreg_buff…

af99829

…er_ctx`

fix: avoid uploading blocks too frequently and splitting large buffers

616f14a

Fixes emqx#13069 (comment)

chore: bump app vsns

9a1d3ea

refactor: move min_block_size one level up

4859ceb

fix: check max_block_size for direct transfers

347971e

docs: fix docs

efa4432

thalesmg force-pushed the azure-blob-storage-action-m-20240508 branch from a92eb3c to efa4432 Compare May 31, 2024 14:10

zmstone approved these changes May 31, 2024

View reviewed changes

thalesmg merged commit c554754 into emqx:master Jun 3, 2024
175 checks passed

thalesmg deleted the azure-blob-storage-action-m-20240508 branch June 3, 2024 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement azure blob storage action #13069

feat: implement azure blob storage action #13069

thalesmg commented May 17, 2024 •

edited

Loading

keynslug left a comment

keynslug May 24, 2024

thalesmg May 28, 2024

keynslug May 24, 2024

keynslug May 24, 2024

thalesmg May 28, 2024

keynslug left a comment

keynslug May 29, 2024

thalesmg May 29, 2024

thalesmg May 29, 2024

zmstone May 31, 2024

thalesmg May 31, 2024

thalesmg May 31, 2024

feat: implement azure blob storage action #13069

feat: implement azure blob storage action #13069

Conversation

thalesmg commented May 17, 2024 • edited Loading

Summary

PR Checklist

Checklist for CI (.github/workflows) changes

keynslug 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

keynslug 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

thalesmg commented May 17, 2024 •

edited

Loading