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

snowflake-destination: allow buffer to be increased till 50 #24698

Merged
merged 4 commits into from Mar 30, 2023

Conversation

subodh1810
Copy link
Contributor

@subodh1810 subodh1810 requested a review from a team as a code owner March 30, 2023 13:42
@subodh1810 subodh1810 self-assigned this Mar 30, 2023
@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Mar 30, 2023
@subodh1810
Copy link
Contributor Author

subodh1810 commented Mar 30, 2023

/test connector=connectors/destination-snowflake

🕑 connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/4565103407
✅ connectors/destination-snowflake https://github.com/airbytehq/airbyte/actions/runs/4565103407
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                            2      0   100%
normalization/transform_catalog/reserved_keywords.py                 15      0   100%
normalization/transform_catalog/__init__.py                           2      0   100%
normalization/destination_type.py                                    18      0   100%
normalization/__init__.py                                             4      0   100%
normalization/transform_catalog/destination_name_transformer.py     171     10    94%
normalization/transform_catalog/table_name_registry.py              174     34    80%
normalization/transform_config/transform.py                         195     48    75%
normalization/transform_catalog/utils.py                             51     14    73%
normalization/transform_catalog/dbt_macro.py                         22      7    68%
normalization/transform_catalog/catalog_processor.py                147     80    46%
normalization/transform_catalog/transform.py                         65     39    40%
normalization/transform_catalog/stream_processor.py                 595    400    33%
-------------------------------------------------------------------------------------
TOTAL                                                              1461    632    57%

Build Passed

Test summary info:

All Passed

@github-actions
Copy link
Contributor

github-actions bot commented Mar 30, 2023

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

  • Run integration tests
  • Bump connector or module version
  • Add changelog
  • Publish the new version

✅ Sources (0)

Connector Version Changelog Publish
  • See "Actionable Items" below for how to resolve warnings and errors.

✅ Destinations (1)

Connector Version Changelog Publish
destination-snowflake 0.4.60
  • See "Actionable Items" below for how to resolve warnings and errors.

👀 Other Modules (1)

  • base-normalization

Actionable Items

(click to expand)

Category Status Actionable Item
Version
mismatch
The version of the connector is different from its normal variant. Please bump the version of the connector.

doc not found
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.
Changelog
doc not found
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug.

changelog missing
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog.
Publish
not in seed
The connector is not in the seed file (e.g. source_definitions.yaml), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug.

diff seed version
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version.

Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

we should probably prioritize decoupling buffer flushes from checkpoints at some point, but this is a good workaround for now

int numOfFileBuffers = FileBuffer.DEFAULT_MAX_CONCURRENT_STREAM_IN_BUFFER;
if (config.has(FileBuffer.FILE_BUFFER_COUNT_KEY)) {
numOfFileBuffers = Math.min(config.get(FileBuffer.FILE_BUFFER_COUNT_KEY).asInt(), FileBuffer.MAX_CONCURRENT_STREAM_IN_BUFFER);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking but maybe a ticket should be filed to add this warning as well here

if (numberOfFileBuffers > FileBuffer.SOFT_CAP_CONCURRENT_STREAM_IN_BUFFER) {
LOGGER.warn("""
Increasing the number of file buffers past {} can lead to increased performance but
leads to increased memory usage. If the number of file buffers exceeds the number
of streams {} this will create more buffers than necessary, leading to nonexistent gains
""", FileBuffer.SOFT_CAP_CONCURRENT_STREAM_IN_BUFFER, catalog.getStreams().size());
}

It's to safeguard users from running into OOM issues by logging the the increased memory size that comes with more FileBuffers

Copy link
Contributor

@ryankfu ryankfu left a comment

Choose a reason for hiding this comment

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

LGTM, non-blocking comment about a LOGGER.warn and maybe add a test but this can be separate

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Mar 30, 2023
@subodh1810
Copy link
Contributor Author

subodh1810 commented Mar 30, 2023

/publish connector=connectors/destination-snowflake

🕑 Publishing the following connectors:
connectors/destination-snowflake
https://github.com/airbytehq/airbyte/actions/runs/4567631522


Connector Did it publish? Were definitions generated?
connectors/destination-snowflake

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@subodh1810 subodh1810 merged commit c2f5d49 into master Mar 30, 2023
18 of 20 checks passed
@subodh1810 subodh1810 deleted the allow-snowflake-file-buffer-50 branch March 30, 2023 19:23
@dimoschi
Copy link

dimoschi commented Apr 2, 2023

How can we use that feature to increase the buffer size?

@subodh1810
Copy link
Contributor Author

@dimoschi upgrade snowflake destination to the latest version. Once done go to the snowflake connector setup page and in the UI you will see this option, you can increase it till 50.
Screenshot 2023-03-31 at 12 49 58 AM

@dimoschi
Copy link

dimoschi commented Apr 3, 2023

@dimoschi upgrade snowflake destination to the latest version. Once done go to the snowflake connector setup page and in the UI you will see this option, you can increase it till 50.

Screenshot 2023-03-31 at 12 49 58 AM

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/destination/snowflake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants