-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Feature/kafkaoptions kwargs #4902
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
base: master
Are you sure you want to change the base?
Changes from all commits
29d13d0
e75d483
57e773f
555c614
1ef5d0a
897df59
394fba0
0a1f724
3de66d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -42,11 +42,13 @@ def __init__( | |||||
message_format: StreamFormat, | ||||||
topic: str, | ||||||
watermark_delay_threshold: Optional[timedelta] = None, | ||||||
kafka_settings: Optional[Dict[str, str]] = None, | ||||||
): | ||||||
self.kafka_bootstrap_servers = kafka_bootstrap_servers | ||||||
self.message_format = message_format | ||||||
self.topic = topic | ||||||
self.watermark_delay_threshold = watermark_delay_threshold or None | ||||||
self.kafka_settings = kafka_settings or None | ||||||
|
||||||
@classmethod | ||||||
def from_proto(cls, kafka_options_proto: DataSourceProto.KafkaOptions): | ||||||
|
@@ -66,11 +68,15 @@ def from_proto(cls, kafka_options_proto: DataSourceProto.KafkaOptions): | |||||
if kafka_options_proto.watermark_delay_threshold.ToNanoseconds() == 0 | ||||||
else kafka_options_proto.watermark_delay_threshold.ToTimedelta() | ||||||
) | ||||||
|
||||||
kafka_options = cls( | ||||||
kafka_bootstrap_servers=kafka_options_proto.kafka_bootstrap_servers, | ||||||
message_format=StreamFormat.from_proto(kafka_options_proto.message_format), | ||||||
topic=kafka_options_proto.topic, | ||||||
watermark_delay_threshold=watermark_delay_threshold, | ||||||
kafka_settings=kafka_options_proto.kafka_settings | ||||||
if kafka_options_proto.HasField("kafka_settings") | ||||||
else None, | ||||||
) | ||||||
|
||||||
return kafka_options | ||||||
|
@@ -92,6 +98,7 @@ def to_proto(self) -> DataSourceProto.KafkaOptions: | |||||
message_format=self.message_format.to_proto(), | ||||||
topic=self.topic, | ||||||
watermark_delay_threshold=watermark_delay_threshold, | ||||||
kafka_settings=self.kafka_settings, | ||||||
) | ||||||
|
||||||
return kafka_options_proto | ||||||
|
@@ -435,6 +442,7 @@ def __init__( | |||||
owner: Optional[str] = "", | ||||||
batch_source: Optional[DataSource] = None, | ||||||
watermark_delay_threshold: Optional[timedelta] = None, | ||||||
kafka_settings: Optional[Dict[str, str]] = None | ||||||
): | ||||||
""" | ||||||
Creates a KafkaSource object. | ||||||
|
@@ -458,11 +466,12 @@ def __init__( | |||||
batch_source (optional): The datasource that acts as a batch source. | ||||||
watermark_delay_threshold (optional): The watermark delay threshold for stream data. | ||||||
Specifically how late stream data can arrive without being discarded. | ||||||
kafka_settings (optional): Optional kafka settings | ||||||
""" | ||||||
if bootstrap_servers: | ||||||
warnings.warn( | ||||||
( | ||||||
"The 'bootstrap_servers' parameter has been deprecated in favor of 'kafka_bootstrap_servers'. " | ||||||
"The 'bootstrap_servers' parameter has been deprecated in favour of 'kafka_bootstrap_servers'. " | ||||||
"Feast 0.25 and onwards will not support the 'bootstrap_servers' parameter." | ||||||
), | ||||||
DeprecationWarning, | ||||||
|
@@ -487,6 +496,7 @@ def __init__( | |||||
message_format=message_format, | ||||||
topic=topic, | ||||||
watermark_delay_threshold=watermark_delay_threshold, | ||||||
kafka_settings=kafka_settings | ||||||
) | ||||||
|
||||||
def __eq__(self, other): | ||||||
|
@@ -542,6 +552,8 @@ def from_proto(data_source: DataSourceProto): | |||||
if data_source.batch_source | ||||||
else None | ||||||
), | ||||||
kafka_settings=(DataSource.from_proto(data_source.kafka_options.kafka_settings) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect method call. kafka_settings should be used directly as a dictionary, not converted using DataSource.from_proto() which expects a DataSource protobuf message.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
if data_source.kafka_options.kafka_settings else None ) | ||||||
) | ||||||
|
||||||
def _to_proto_impl(self) -> DataSourceProto: | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -221,6 +221,7 @@ class DataSource(google.protobuf.message.Message): | |
TOPIC_FIELD_NUMBER: builtins.int | ||
MESSAGE_FORMAT_FIELD_NUMBER: builtins.int | ||
WATERMARK_DELAY_THRESHOLD_FIELD_NUMBER: builtins.int | ||
KAFKA_SETTINGS_FIELD_NUMBER: builtins.int | ||
kafka_bootstrap_servers: builtins.str | ||
"""Comma separated list of Kafka bootstrap servers. Used for feature tables without a defined source host[:port]]""" | ||
topic: builtins.str | ||
|
@@ -231,16 +232,20 @@ class DataSource(google.protobuf.message.Message): | |
@property | ||
def watermark_delay_threshold(self) -> google.protobuf.duration_pb2.Duration: | ||
"""Watermark delay threshold for stream data""" | ||
@property | ||
def kafka_settings(self) -> feast.core.DataFormat_pb2.StreamFormat: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The unit tests are failing with the kafka_settings being of type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the code is rebased, the link mentioned here may point to a different code. So being particular here: |
||
"""kafka_settings field""" | ||
Comment on lines
+236
to
+237
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return type for kafka_settings is incorrect. It should be a dictionary-like type for key-value pairs, not StreamFormat. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
def __init__( | ||
self, | ||
*, | ||
kafka_bootstrap_servers: builtins.str = ..., | ||
topic: builtins.str = ..., | ||
message_format: feast.core.DataFormat_pb2.StreamFormat | None = ..., | ||
watermark_delay_threshold: google.protobuf.duration_pb2.Duration | None = ..., | ||
kafka_settings: feast.core.DataFormat_pb2.StreamFormat | None = ..., | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type annotation for kafka_settings parameter is incorrect. It should be a dictionary-like type for key-value pairs, not StreamFormat. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
) -> None: ... | ||
def HasField(self, field_name: typing_extensions.Literal["message_format", b"message_format", "watermark_delay_threshold", b"watermark_delay_threshold"]) -> builtins.bool: ... | ||
def ClearField(self, field_name: typing_extensions.Literal["kafka_bootstrap_servers", b"kafka_bootstrap_servers", "message_format", b"message_format", "topic", b"topic", "watermark_delay_threshold", b"watermark_delay_threshold"]) -> None: ... | ||
def HasField(self, field_name: typing_extensions.Literal["kafka_settings", b"kafka_settings", "message_format", b"message_format", "watermark_delay_threshold", b"watermark_delay_threshold"]) -> builtins.bool: ... | ||
def ClearField(self, field_name: typing_extensions.Literal["kafka_bootstrap_servers", b"kafka_bootstrap_servers", "kafka_settings", b"kafka_settings", "message_format", b"message_format", "topic", b"topic", "watermark_delay_threshold", b"watermark_delay_threshold"]) -> None: ... | ||
|
||
class KinesisOptions(google.protobuf.message.Message): | ||
"""Defines options for DataSource that sources features from Kinesis records. | ||
|
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.
Use American English spelling 'favor' instead of British English 'favour' for consistency with the codebase.
Copilot uses AI. Check for mistakes.