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.

Sign up for GitHub

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

feat: add reasons to AirbyteStreamStatusTraceMessage #74

Merged
merged 7 commits into from
May 16, 2024
Merged

feat: add reasons to AirbyteStreamStatusTraceMessage #74

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
777b9a4
feat: add RATE_LIMITED enum + metadata to AirbyteStreamStatusTraceMes…
subodh1810 May 3, 2024
ab3e302
use timestamp instead of metadata
subodh1810 May 3, 2024
684fcab
til instead of till
subodh1810 May 3, 2024
e2c7ad4
update protocol changes according to discussion
subodh1810 May 8, 2024
b192acd
Merge branch 'main' into add-rate-limited-enum-trace-message
subodh1810 May 8, 2024
c6523f6
use integer instead of number
subodh1810 May 13, 2024
c76a2f9
add TODOs
subodh1810 May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,30 @@ definitions:
- RUNNING # Stream has read its first byte/message
- COMPLETE # Stream has completed executing without interruption or error
- INCOMPLETE # Stream has stopped due to an interruption or error
AirbyteStreamStatusReasonType:
type: string
description: >
Type of reason
enum:
- RATE_LIMITED
AirbyteStreamStatusRateLimitedReason:
type: object
description: Rate Limited Information
properties:
quota_reset:
description: "Optional time in ms representing when the API quota is going to be reset"
type: number
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason this isn't just a big int? do we need decimals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgardens I am just following the convention. We are using number for timestamp attributes. For instance take a look at the emitted_at in AirbyteTraceMessage, its defined as a number

      emitted_at:
        description: "the time in ms that the message was emitted"
        type: number

Copy link
Contributor

Choose a reason for hiding this comment

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

@cgardens @subodh1810 This looks like a bug in the AirbyteTraceMessage. It was not obvious to me that integer was not an integer data type and is actually a big integer or that number is a floating point/double. Should we attempt to fix the AirbyteTraceMessage either as part of this PR or in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • let's do it correctly here and use integer.
  • let's create an issue to fix airbyte trace meesage and add a todo in the code that that should have been number. don't think this is a good moment to try to fix this old mistake. just want to do it right going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgardens @jdpgrailsdev added the TODOs in the protocol yaml and created a github issue
https://github.com/airbytehq/airbyte/issues/38163

AirbyteStreamStatusReason:
type: object
required:
- type
description: >
The reason associated with the status of the stream.
properties:
type:
"$ref": "#/definitions/AirbyteStreamStatusReasonType"
rate_limited:
"$ref": "#/definitions/AirbyteStreamStatusRateLimitedReason"
AirbyteStreamStatusTraceMessage:
type: object
additionalProperties: true
Expand All @@ -328,6 +352,11 @@ definitions:
status:
description: "The current status of the stream"
"$ref": "#/definitions/AirbyteStreamStatus"
reasons:
description: "The reasons associated with the status of the stream"
type: array
items:
"$ref": "#/definitions/AirbyteStreamStatusReason"
AirbyteAnalyticsTraceMessage:
type: object
additionalProperties: true
Expand Down
29 changes: 29 additions & 0 deletions protocol-models/src/main/resources/airbyte_protocol/v0/airbyte_protocol.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,30 @@ definitions:
- RUNNING # Stream has read its first byte/message
- COMPLETE # Stream has completed executing without interruption or error
- INCOMPLETE # Stream has stopped due to an interruption or error
AirbyteStreamStatusReasonType:
type: string
description: >
Type of reason
enum:
- RATE_LIMITED
AirbyteStreamStatusRateLimitedReason:
type: object
description: Rate Limited Information
properties:
quota_reset:
description: "Optional time in ms representing when the API quota is going to be reset"
type: number
AirbyteStreamStatusReason:
type: object
required:
- type
description: >
The reason associated with the status of the stream.
properties:
type:
"$ref": "#/definitions/AirbyteStreamStatusReasonType"
rate_limited:
"$ref": "#/definitions/AirbyteStreamStatusRateLimitedReason"
AirbyteStreamStatusTraceMessage:
type: object
additionalProperties: true
Expand All @@ -329,6 +353,11 @@ definitions:
status:
description: "The current status of the stream"
"$ref": "#/definitions/AirbyteStreamStatus"
reasons:
description: "The reasons associated with the status of the stream"
type: array
items:
"$ref": "#/definitions/AirbyteStreamStatusReason"
AirbyteAnalyticsTraceMessage:
type: object
additionalProperties: true
Expand Down
Loading