Skip to content
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

fixes #1062 : removed empty aws client credentials and set default region #1063

Closed
wants to merge 1 commit into from

Conversation

jguittard
Copy link
Contributor

Change Log

Added

Fixed

  • removed empty aws client credentials and set default region

Changed

Removed

Deprecated

Security


Description

Copy link
Contributor

Flow PHP - Benchmarks

Results of the benchmarks from this PR are compared with the results from 1.x branch.

Extractors
+-----------------------+-------------------+------+-----+------------------+------------------+----------------+
| benchmark             | subject           | revs | its | mem_peak         | mode             | rstdev         |
+-----------------------+-------------------+------+-----+------------------+------------------+----------------+
| AvroExtractorBench    | bench_extract_10k | 1    | 3   | 35.422mb +0.00%  | 838.400ms +0.45% | ±0.09% -63.12% |
| CSVExtractorBench     | bench_extract_10k | 1    | 3   | 5.142mb +0.00%   | 340.697ms +0.15% | ±0.86% -68.40% |
| JsonExtractorBench    | bench_extract_10k | 1    | 3   | 5.173mb +0.00%   | 1.077s -1.42%    | ±0.06% -94.37% |
| ParquetExtractorBench | bench_extract_10k | 1    | 3   | 135.845mb +0.00% | 913.251ms +0.23% | ±0.45% -29.59% |
| TextExtractorBench    | bench_extract_10k | 1    | 3   | 4.933mb +0.00%   | 35.821ms -0.14%  | ±0.91% +35.32% |
| XmlExtractorBench     | bench_extract_10k | 1    | 3   | 4.939mb +0.00%   | 433.565ms -0.53% | ±0.37% -41.05% |
+-----------------------+-------------------+------+-----+------------------+------------------+----------------+
Transformers
+-----------------------------+--------------------------+------+-----+------------------+-----------------+----------------+
| benchmark                   | subject                  | revs | its | mem_peak         | mode            | rstdev         |
+-----------------------------+--------------------------+------+-----+------------------+-----------------+----------------+
| RenameEntryTransformerBench | bench_transform_10k_rows | 1    | 3   | 116.239mb +0.00% | 62.132ms +2.44% | ±0.65% +54.56% |
+-----------------------------+--------------------------+------+-----+------------------+-----------------+----------------+
Loaders
+--------------------+----------------+------+-----+------------------+------------------+-----------------+
| benchmark          | subject        | revs | its | mem_peak         | mode             | rstdev          |
+--------------------+----------------+------+-----+------------------+------------------+-----------------+
| AvroLoaderBench    | bench_load_10k | 1    | 3   | 96.807mb +0.00%  | 455.558ms -1.74% | ±0.58% +140.90% |
| CSVLoaderBench     | bench_load_10k | 1    | 3   | 55.221mb +0.00%  | 68.078ms -1.57%  | ±0.49% +269.57% |
| JsonLoaderBench    | bench_load_10k | 1    | 3   | 107.593mb +0.00% | 50.832ms -1.76%  | ±1.07% +118.22% |
| ParquetLoaderBench | bench_load_10k | 1    | 3   | 227.013mb +0.00% | 1.416s -0.27%    | ±1.07% +231.07% |
| TextLoaderBench    | bench_load_10k | 1    | 3   | 17.976mb +0.00%  | 39.052ms -1.37%  | ±0.37% -53.91%  |
+--------------------+----------------+------+-----+------------------+------------------+-----------------+
Building Blocks
+-------------------------+----------------------------+------+-----+------------------+------------------+------------------+
| benchmark               | subject                    | revs | its | mem_peak         | mode             | rstdev           |
+-------------------------+----------------------------+------+-----+------------------+------------------+------------------+
| RowsBench               | bench_chunk_10_on_10k      | 2    | 3   | 87.060mb +0.00%  | 3.427ms -2.99%   | ±2.34% -17.56%   |
| RowsBench               | bench_diff_left_1k_on_10k  | 2    | 3   | 102.658mb +0.00% | 190.430ms -0.04% | ±0.66% -42.17%   |
| RowsBench               | bench_diff_right_1k_on_10k | 2    | 3   | 85.378mb +0.00%  | 19.102ms +0.75%  | ±2.50% +1358.65% |
| RowsBench               | bench_drop_1k_on_10k       | 2    | 3   | 88.300mb +0.00%  | 1.787ms -0.09%   | ±3.53% +83.09%   |
| RowsBench               | bench_drop_right_1k_on_10k | 2    | 3   | 88.300mb +0.00%  | 1.705ms -5.67%   | ±1.91% -10.81%   |
| RowsBench               | bench_entries_on_10k       | 2    | 3   | 85.412mb +0.00%  | 2.632ms -2.69%   | ±1.51% -48.59%   |
| RowsBench               | bench_filter_on_10k        | 2    | 3   | 85.941mb +0.00%  | 16.515ms +3.27%  | ±1.46% +1.84%    |
| RowsBench               | bench_find_on_10k          | 2    | 3   | 85.941mb +0.00%  | 16.102ms +0.23%  | ±0.59% -36.20%   |
| RowsBench               | bench_find_one_on_10k      | 10   | 3   | 83.845mb +0.00%  | 1.706μs 0.00%    | ±2.72% 0.00%     |
| RowsBench               | bench_first_on_10k         | 10   | 3   | 83.845mb +0.00%  | 0.400μs +33.33%  | ±0.00% +0.00%    |
| RowsBench               | bench_flat_map_on_1k       | 2    | 3   | 93.195mb +0.00%  | 12.665ms +0.29%  | ±0.96% -27.32%   |
| RowsBench               | bench_map_on_10k           | 2    | 3   | 122.566mb +0.00% | 60.550ms -2.53%  | ±0.55% -10.82%   |
| RowsBench               | bench_merge_1k_on_10k      | 2    | 3   | 86.460mb +0.00%  | 1.409ms +11.15%  | ±0.40% -85.51%   |
| RowsBench               | bench_partition_by_on_10k  | 2    | 3   | 89.807mb +0.00%  | 64.739ms -1.26%  | ±1.23% +144.49%  |
| RowsBench               | bench_remove_on_10k        | 2    | 3   | 88.562mb +0.00%  | 3.984ms +0.28%   | ±2.88% +221.73%  |
| RowsBench               | bench_sort_asc_on_1k       | 2    | 3   | 83.988mb +0.00%  | 40.277ms -1.64%  | ±0.63% -79.09%   |
| RowsBench               | bench_sort_by_on_1k        | 2    | 3   | 83.989mb +0.00%  | 40.359ms +0.60%  | ±1.30% -21.72%   |
| RowsBench               | bench_sort_desc_on_1k      | 2    | 3   | 83.988mb +0.00%  | 40.508ms -1.93%  | ±2.01% +3.12%    |
| RowsBench               | bench_sort_entries_on_1k   | 2    | 3   | 86.286mb +0.00%  | 7.269ms -1.63%   | ±0.11% -73.70%   |
| RowsBench               | bench_sort_on_1k           | 2    | 3   | 83.845mb +0.00%  | 29.328ms +0.54%  | ±0.46% -43.04%   |
| RowsBench               | bench_take_1k_on_10k       | 10   | 3   | 83.845mb +0.00%  | 13.701μs -5.29%  | ±2.64% -8.05%    |
| RowsBench               | bench_take_right_1k_on_10k | 10   | 3   | 83.845mb +0.00%  | 16.036μs -3.67%  | ±1.75% -50.78%   |
| RowsBench               | bench_unique_on_1k         | 2    | 3   | 102.659mb +0.00% | 191.316ms +0.52% | ±0.58% -18.73%   |
| NativeEntryFactoryBench | bench_entry_factory        | 1    | 3   | 116.789mb +0.00% | 505.761ms -0.40% | ±0.51% -56.62%   |
| NativeEntryFactoryBench | bench_entry_factory        | 1    | 3   | 60.267mb +0.00%  | 251.369ms -0.89% | ±0.51% +40.74%   |
| NativeEntryFactoryBench | bench_entry_factory        | 1    | 3   | 15.201mb +0.00%  | 54.179ms -0.29%  | ±0.75% -74.32%   |
| TypeDetectorBench       | bench_type_detector        | 1    | 3   | 59.975mb +0.00%  | 434.704ms +0.22% | ±0.47% +30.81%   |
| TypeDetectorBench       | bench_type_detector        | 1    | 3   | 14.514mb +0.00%  | 86.380ms -0.23%  | ±0.82% +45.34%   |
+-------------------------+----------------------------+------+-----+------------------+------------------+------------------+

'secret' => '',
],
'region' => '',
'region' => 'us-east-1',
Copy link
Member

Choose a reason for hiding this comment

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

I'm honestly not sure about default region, I think I would rather prefer to throw an exception when the region is not set. Is there anything that makes us-east-1 special enough to make it default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just because it's the default region AWS applies.
IMHO I'd rather avoid merging any default configuration :)

Copy link
Member

Choose a reason for hiding this comment

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

IMHO I'd rather avoid merging any default configuration :)

Same, can't say out of my head why it was implemented this way, I need to take another look later in the codebase but as far as I remember there was a reason. I will look into this later and get back to you

Copy link
Contributor Author

@jguittard jguittard Apr 29, 2024

Choose a reason for hiding this comment

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

Simple : S3 client needs an array with at least region and version set. If $contextOptions does not provide it, it will throw an exception within aws-sdk. Maybe check those 2 keys right before instantiating Filesystem and throw a domain exception instead?

Copy link
Member

Choose a reason for hiding this comment

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

I was playing with it a bit and I think we can skip any defaults, when region or credentials are missing, S3 client will throw an exception which I think is good since it's S3 responsibility, not flow.
Could you remove those merges from both, S3 and Azure clients and in the meantime I'm gonna prepare some examples for the docs that would explain how to use them both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it. I'll take care of it tomorrow

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, examples should be available any moment: #1065

Copy link
Member

Choose a reason for hiding this comment

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

@jguittard would you mind if I would take it over from here and adjust your PR so it can be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants