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

Add support for stacked sinks #611

Merged
merged 3 commits into from
Dec 6, 2024
Merged

Conversation

saiharshavellanki
Copy link
Contributor

@saiharshavellanki saiharshavellanki commented Dec 6, 2024

  • Add new parameter stacked in sink that is set as None by default and customers should set it as True for all the new sinks added in future on top of same dataset
  • Updated documentation accordingly
Screenshot 2024-12-06 at 4 29 53 PM Screenshot 2024-12-06 at 4 30 00 PM Screenshot 2024-12-06 at 4 32 45 PM

Important

Adds support for stacked sinks with a new stacked parameter in sink() and updates validation and documentation accordingly.

  • Behavior:
    • Adds stacked parameter to sink() in connectors.py, defaulting to None. Users should set it to True for additional sinks on the same dataset.
    • Validates stacked sinks in to_sync_request_proto() in to_proto.py.
    • Raises errors for incorrect stacked sink configurations in test_invalid_connectors.py.
  • Documentation:
    • Updates sink.md and sink.py to document the stacked parameter and its usage.
  • Misc:
    • Updates connector_pb2.py and connector_pb2.pyi to include stacked field in Sink message.
    • Bumps version to 1.5.59 in pyproject.toml.

This description was created by Ellipsis for fba3198. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 7aece6e in 1 minute and 15 seconds

More details
  • Looked at 641 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. fennel/internal_lib/to_proto/to_proto.py:195
  • Draft comment:
    The validation logic for stacked sinks could be simplified and the error messages could be more informative. Consider refactoring the logic to improve readability and provide more context in the error messages.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in fennel/internal_lib/to_proto/to_proto.py has a potential issue with the validation logic for stacked sinks. The logic checks for the number of stacked sinks and raises errors if the conditions are not met. However, the error messages could be more informative and the logic could be simplified for better readability.

Workflow ID: wflow_UR2m1O4db6n2s4xy


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on af1e6ab in 37 seconds

More details
  • Looked at 642 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. fennel/connectors/connectors.py:258
  • Draft comment:
    Consider setting the default value of stacked to False instead of None for better clarity and to align with typical expectations.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new parameter stacked for sinks, but the default value is set to None. It would be more intuitive to set the default to False, as this would align with the typical expectation that a sink is not stacked unless explicitly specified.

Workflow ID: wflow_SUvwQUplGV21k4pb


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 7b6a57f in 43 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_YyTdt2PtHMPOL4Sr


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

docs/examples/concepts/sink.py Outdated Show resolved Hide resolved
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on fc79745 in 16 seconds

More details
  • Looked at 45 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. docs/examples/concepts/sink.py:18
  • Draft comment:
    The import statements for Kafka, S3, eval, and source are repeated inside the test_stacked_sinks function. Consider moving them to the top of the file to avoid redundancy and improve readability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statements for Kafka, S3, eval, and source are repeated inside the test_stacked_sinks function. These should be moved to the top of the file to avoid redundancy and improve readability.

Workflow ID: wflow_mMqnDNGLzv23EWPX


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on fba3198 in 27 seconds

More details
  • Looked at 49 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. docs/examples/concepts/sink.py:19
  • Draft comment:
    Consider moving the import statements for Kafka, Snowflake, and sink to the top of the file to avoid redundancy and improve readability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statements for Kafka, Snowflake, and sink are repeated within the function test_stacked_sinks. This is unnecessary and can be moved to the top of the file to avoid redundancy and improve readability.

Workflow ID: wflow_XSe7iapnm7MiSYvy


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@saiharshavellanki saiharshavellanki merged commit 725eb40 into main Dec 6, 2024
8 checks passed
@saiharshavellanki saiharshavellanki deleted the harsha/stacked_sinks branch December 6, 2024 14:24
saiharshavellanki added a commit that referenced this pull request Dec 6, 2024
saiharshavellanki added a commit that referenced this pull request Dec 6, 2024
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.

2 participants