You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "FMX (via GitHub)" <gi...@apache.org> on 2024/01/04 02:02:48 UTC
[PR] [CELEBORN-1211] Add extension instead of encryption [incubator-celeborn]
FMX opened a new pull request, #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206
### What changes were proposed in this pull request?
1. Remove encryption from Celeborn.
2. Add extension API to CelebornShuffleHandler.
### Why are the changes needed?
Remove encryption for legal reasons.
### Does this PR introduce _any_ user-facing change?
NO.
### How was this patch tested?
GA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#issuecomment-1877549831
Can you provide more context about the legal reasons please ?
I did not find any details in the jira CELEBORN-1211 as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#issuecomment-1876267212
would the change be clearer if we do a clean revert first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#discussion_r1441610914
##########
client/src/main/java/org/apache/celeborn/client/ShuffleClient.java:
##########
@@ -122,6 +134,8 @@ public static void printReadStats(Logger logger) {
public abstract void setupLifecycleManagerRef(RpcEndpointRef endpointRef);
+ public abstract void setExtension(byte[] extension);
Review Comment:
can we have docs for that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Add extension instead of encryption [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#issuecomment-1876217916
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2206?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`b94fea8`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/b94fea8e176334f120526e01bfd0654251e26c46?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.13% compared to head [(`0701fca`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2206?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 47.08%.
> Report is 1 commits behind head on main.
> :exclamation: Current head 0701fca differs from pull request most recent head 434775f. Consider uploading reports for the commit 434775f to get more accurate results
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2206 +/- ##
==========================================
- Coverage 47.13% 47.08% -0.05%
==========================================
Files 189 189
Lines 11732 11711 -21
Branches 1046 1046
==========================================
- Hits 5529 5513 -16
+ Misses 5833 5829 -4
+ Partials 370 369 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2206?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#issuecomment-1878128392
Sounds good, btw going through the process should not be very cumbersome - we can get help from folks who have done it in past at Apache projects :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#discussion_r1441252467
##########
client/src/main/java/org/apache/celeborn/client/ShuffleClient.java:
##########
@@ -142,13 +130,12 @@ public static void printReadStats(Logger logger) {
String.format("%.2f", (localReadCount * 1.0d / totalReadCount) * 100));
}
- public void setupIoCrypto(
- Optional<byte[]> ioCryptoKey, Properties ioCryptoConf, byte[] ioCryptoInitializationVector) {}
-
public abstract void setupLifecycleManagerRef(String host, int port);
public abstract void setupLifecycleManagerRef(RpcEndpointRef endpointRef);
+ public abstract void setupExtension(byte[] extension);
Review Comment:
Better to use `setExtension`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#issuecomment-1876279109
> would the change be clearer if we do a clean revert first?
I think you are right. I suggested not to revert yesterday, but now I think revert is clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#issuecomment-1877981800
> Can you provide more context about the legal reasons please ? I did not find any details in the jira [CELEBORN-1211](https://issues.apache.org/jira/browse/CELEBORN-1211) as well.
Hi @mridulm , we found instructions about adding cryptography features in this doc:
https://infra.apache.org/crypto.html
and we decided not to include such code for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Use extension instead of encryption [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2206:
URL: https://github.com/apache/incubator-celeborn/pull/2206#issuecomment-1878155854
> Sounds good, btw going through the process should not be very cumbersome - we can get help from folks who have done it in past at Apache projects :-)
That will be great. The reverted crypto logic was only for our internal use, if it's required by community in the future, we can discuss about the process :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1211] Add extension for celeborn shuffle handler [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2206: [CELEBORN-1211] Add extension for celeborn shuffle handler
URL: https://github.com/apache/incubator-celeborn/pull/2206
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org