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

Hello Conversion Layer Adapter #1976

Merged
merged 23 commits into from
Jan 2, 2024
Merged

Hello Conversion Layer Adapter #1976

merged 23 commits into from
Jan 2, 2024

Conversation

ueman
Copy link
Contributor

@ueman ueman commented Sep 21, 2023

Working towards #1740

After this is published, we can remove the code from the native_dio_adapter and depend on this package instead.

TODO

  • Tests
  • CI

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

@AlexV525 AlexV525 added the s: feature This issue indicates a feature request label Oct 1, 2023
@zhangyaobin

This comment was marked as outdated.

@zhangyaobin

This comment was marked as outdated.

@AlexV525
Copy link
Member

@ueman Is this still on your track?

@ueman
Copy link
Contributor Author

ueman commented Dec 1, 2023

I don't think I have the time right now to work on this. I'm happy to give pointer to anyone who wants to take this over though.

@ueman ueman closed this Dec 1, 2023
@AlexV525 AlexV525 reopened this Dec 2, 2023
@AlexV525
Copy link
Member

AlexV525 commented Dec 2, 2023

I've also requested license and authoring info changes. FYI @ueman

@AlexV525 AlexV525 marked this pull request as ready for review December 2, 2023 08:06
@AlexV525 AlexV525 requested a review from a team as a code owner December 2, 2023 08:06
@AlexV525
Copy link
Member

AlexV525 commented Dec 2, 2023

86b7a7b indicates that we're not using the StreamedRequest for the native_dio_adapter, which will cost bytes amount of memory waste when sending with the request stream. It should be addressed separately before we roll the layer for the adapter.

@ueman
Copy link
Contributor Author

ueman commented Dec 3, 2023

86b7a7b indicates that we're not using the StreamedRequest for the native_dio_adapter, which will cost bytes amount of memory waste when sending with the request stream. It should be addressed separately before we roll the layer for the adapter.

I think we can still release it without converting native_dio_adapter and deal with the StreamedRequest problem later. That way people can already make use of it and report issues. Though, I don't expect many, since people are already using this with native_dio_adapter. The version number also already indicates that it's experimental.

@AlexV525
Copy link
Member

AlexV525 commented Dec 3, 2023

I think we can still release it without converting native_dio_adapter and deal with the StreamedRequest problem later.

Yeah I'm not expecting to convert it in recent weeks.

@AlexV525
Copy link
Member

AlexV525 commented Dec 9, 2023

I've removed the http from its name since we might add more compatibilities to the packages in the future.

Copy link
Contributor Author

@ueman ueman left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this over the finish line

@AlexV525
Copy link
Member

I'll check with @CaiJingLong to see if we need extra support around publishing tools for the new package.

Signed-off-by: Alex Li <[email protected]>
@AlexV525 AlexV525 added the p: compatibility_layer Targeting `compatibility_layer` package label Dec 15, 2023
@AlexV525 AlexV525 disabled auto-merge January 2, 2024 02:36
@AlexV525 AlexV525 merged commit 7822682 into main Jan 2, 2024
3 checks passed
@AlexV525 AlexV525 deleted the conversion-layer-to-http branch January 2, 2024 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: compatibility_layer Targeting `compatibility_layer` package s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants