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

Jump to bottom

[airbyte-cdk] - Update JSON Error Message Parser to return additional error message fields or default to dict #44010

Merged
merged 7 commits into from
Aug 28, 2024
Merged

[airbyte-cdk] - Update JSON Error Message Parser to return additional error message fields or default to dict #44010

merged 7 commits into from
Aug 28, 2024

Conversation

pnilan
Copy link
Contributor

@pnilan pnilan commented Aug 13, 2024 •
edited
Loading

What

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@vercel Vercel
Copy link

vercel bot commented Aug 13, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Aug 28, 2024 5:53pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Aug 13, 2024
@natikgadzhi
Copy link
Contributor

@alafanechere note: CDK changes regression tests are failing here: https://fff942bf5099f1330556ce7fdc9c8473e612d2cc255905901f6214e-apidata.googleusercontent.com/download/storage/v1/b/airbyte-ci-reports-multi/o/airbyte-ci%2Fconnectors%2Ftest%2Fpull_request%2Fpnilan_airbyte-cdk-improve-error-messages-parsing%2F1723582353%2F24b2e4fa50925f9d42b4636ec86f4435cbba5cd8%2Fsource-shopify%2F2.4.20%2Foutput.html?jk=AXZI6JxzKrkkrA9ZobZjFyIqybP76A3l-aTkgkzFGo8OXrRX5V_er9qRYS29Is-Cfl4pAA8g4W5XJ9LsmkXDLuV05qaJLceuat7hzY0J9iXx9oGr-hpvu5mvSV6JkrKUwiaO53tvLC-5WHwIYKpztrZOKtF180DVyU7r4oy-P-saSohugvWxPNGjbeb1Oo2_5RqfhtTxJajQuWjrSDZUZ8_ctnH8SbGd8XpoRpiN18Z1cb5x23ylrgmTWJGMdCLspZVPOPs8ArhQS_i10HJ134qFZhsAgWge-dmDiBF0gzjXiI31oWr9xEXqdLEDZMx3QlpqtEAu06NVBA6Qgbq0ppR5-PItDOKzjPgyBHHWxVUkfrIi1GipPhl6xBTUTwcIkysGYceUVDBxwVUF-l_y5HSM_zuW2Ax6CDlLC_2m4kfdE-rWirSIixGXZdv3tPbsoIiOnUWTK1kjct7o-ZjylZgMro57__9q0GcJ3MA3wAnFWi7IEDmUkNJJJzDi4rhqhpevEHivYYsGiw5C4hFJ6i-BJ_3ZC2tBFTPkPJ9Y3kTXD6Z4B1RBBeop5AVmyZClPlL-BWMf0BS1C2iZ8ge_gz3-qUEgIhuI-0kqtMMy_s29FbBH-_np4rU1jbKRaGkMJQpOAU6Q-5pWRmW-bAsopEzJj7_IQUYbk0jesHAwsYqC-C7J4c-c5EXXPbFLpvP4VVNiUXm33yocEb3-l5wq3QYXfqwtO23Q1kciAsZQ8U8JHNq0rJtIMysnYszNCkjjbTSUAUxdv6Lxp3X9Ur9mwABMxK2udmKyXQWDc-7MnwofA2jnPNos13YJ7SsCgXws56IkmyH-PcW5oDbZiznTQblO__b2WNgUPW3QsCn5v60omqCs3dA_EcoefDTljSkE4TP3caLGlELhT69N1WaJgH246tsE2cgPNI3hJlij1PrTbHDi3nN9j1K1RgxweousJZJCboxPQSE7c574ZbOoJDHDsdAbbCpxzG2Cjp3Xp_j13suW_k5Nfu1LMx-I3FRUGsyJxedOZgoQUKnxXLpKkivDj3gowtSXoFJcV1E-j4cF-P6uyd87aDrn3jOgt71HWKQR8z14WF0xhuiXU7A_3T9r1jcqbR1DRCuvtxWxwoUXJrrbcEetEAROKLIWirNQ3gEZA-L1IEHgP5H6o9oqKCRbyZcpreFoMXSzqoe_6C-iAMd_wLu7KfNP80BCGUlwZGnEa27ELTLaeLlzdUluJgMfCxazkeoZYpJOHyzSVOvDVM5EREiO1XzBZHySnIVVr5Yput9Bb80tFBRwEsytAOv6GXj4nROzszWNgBUtB4bxY3kJzBNCocr2Upe4Vs96Ny_xGQswkMQCQxD9aL9WYG2PmnxYPRT6Roj6xOtYQbykAnNfnPWkrKPvGM5WQHor2DVB-UpQCDfjIG3WH3gWHz-8Dw&isca=1 says it doesn't have a folder?

@alafanechere
Copy link
Contributor

@alafanechere note: CDK changes regression tests are failing here.

@clnoll I'm not 100% sure. Do we want to skip or run these regression tests on CDK change? If we want to run them there's indeed a bug to fix. I think we should disable them until fixed to not block the python team.

@natikgadzhi
Copy link
Contributor

natikgadzhi commented Aug 14, 2024 via email

girarda
girarda approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

@pnilan can you add some unit tests covering these cases?

codeflash-ai bot added a commit that referenced this pull request Aug 28, 2024
@codeflash-ai
⚡️ Speed up method JsonErrorMessageParser._try_get_error by 25% in PR
33271c8 
#44010 (`pnilan/airbyte-cdk-improve-error-messages-parsing`)

Certainly! To optimize the given code, especially focusing on runtime improvements, we can.

1. Replace the list comprehension and generator used in the `_try_get_error` method with more efficient alternatives.
2. Optimize the dictionary look-up to check keys sequentially rather than getting values multiple times.

Here's the revised code.
@codeflash-ai codeflash-ai bot mentioned this pull request Aug 28, 2024
Merged
@codeflash-ai CodeFlash AI
Copy link

codeflash-ai bot commented Aug 28, 2024

⚡️ Codeflash found optimizations for this PR

📄 JsonErrorMessageParser._try_get_error() in airbyte-cdk/python/airbyte_cdk/sources/streams/http/error_handlers/json_error_message_parser.py

📈 Performance improved by 25% (0.25x faster)

⏱️ Runtime went down from 677 microseconds to 543 microseconds

I created a new dependent PR with the suggested changes. Please review:

If you approve, it will be merged into this PR (branch pnilan/airbyte-cdk-improve-error-messages-parsing).

@pnilan
Copy link
Contributor Author

pnilan commented Aug 28, 2024 •
edited by github-actions bot
Loading

/approve-regression-tests

Check job output.

✅ Approving regression tests

@codeflash-ai CodeFlash AI
Copy link

codeflash-ai bot commented Aug 28, 2024

This PR is now faster! 🚀 @pnilan accepted my optimizations from:

@codeflash-ai CodeFlash AI
Copy link

codeflash-ai bot commented Aug 28, 2024

This PR is now faster! 🚀 codeflash-ai[bot] accepted my code suggestion above.

@CLAassistant
Copy link

CLAassistant commented Aug 28, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@pnilan pnilan force-pushed the pnilan/airbyte-cdk-improve-error-messages-parsing branch from 85960ad to 13f48c9 Compare August 28, 2024 17:55
@pnilan
Copy link
Contributor Author

pnilan commented Aug 28, 2024 •
edited by github-actions bot
Loading

/approve-regression-tests

Check job output.

✅ Approving regression tests

@pnilan pnilan merged commit 1060161 into master Aug 28, 2024
57 of 59 checks passed
@pnilan pnilan deleted the pnilan/airbyte-cdk-improve-error-messages-parsing branch August 28, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@girarda girarda girarda approved these changes

Assignees
No one assigned
Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pnilan @natikgadzhi @alafanechere @CLAassistant @girarda @octavia-squidington-iii