-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore: Rename stream data source parameters #2804
chore: Rename stream data source parameters #2804
Conversation
a3b53fd
to
edf4c3b
Compare
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.
/lgtm
Codecov Report
@@ Coverage Diff @@
## master #2804 +/- ##
==========================================
- Coverage 80.52% 80.50% -0.02%
==========================================
Files 172 172
Lines 15352 15356 +4
==========================================
+ Hits 12362 12363 +1
- Misses 2990 2993 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
/lgtm
kafka_bootstrap_servers: str, | ||
message_format: StreamFormat, | ||
topic: str, | ||
watermark: Optional[timedelta] = None, | ||
watermark_delay_threshold: Optional[timedelta] = None, |
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.
Do we need to worry about backwards compatibility?
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.
no for watermark_delay_threshold
as this was very recently added by Kevin and has not been part of a release yet
yes for kafka_bootstrap_servers
, which I believe I handled appropriately
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, felixwang9817 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sdk/python/feast/data_source.py
Outdated
name: str. Name of data source, which should be unique within a project | ||
event_timestamp_column (optional): str. (Deprecated) Event timestamp column used for point in time | ||
name: Name of data source, which should be unique within a project | ||
event_timestamp_column: (Deprecated) Event timestamp column used for point in time |
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.
event_timestamp_column: (Deprecated) Event timestamp column used for point in time | |
event_timestamp_column: (Deprecated in favor of `timestamp_field`) Event timestamp column used for point in time |
applies everywhere
Signed-off-by: Felix Wang <[email protected]>
Signed-off-by: Felix Wang <[email protected]>
Signed-off-by: Felix Wang <[email protected]>
Signed-off-by: Felix Wang <[email protected]>
Signed-off-by: Felix Wang <[email protected]>
Signed-off-by: Felix Wang <[email protected]>
Signed-off-by: Felix Wang <[email protected]>
2a8afba
to
fc12f54
Compare
/lgtm |
What this PR does / why we need it: This PR renames
bootstrap_servers
tokafka_bootstrap_servers
andwatermark
towatermark_delay_threshold
forKafkaSource
. It also deprecates thedate_partition_column
for all data sources.Which issue(s) this PR fixes:
Fixes #