You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "snleee (via GitHub)" <gi...@apache.org> on 2023/03/30 19:44:00 UTC
[GitHub] [pinot] snleee opened a new pull request, #10511: Allow empty segmentsTo for segment replacement protocol
snleee opened a new pull request, #10511:
URL: https://github.com/apache/pinot/pull/10511
This PR attempts to support the empty segmentsTo
- (s1, s2, s3) -> () is a valid replacement where we want to delete (s1, s2, s3) at the same time.
- We already support () -> (s1, s2, s3)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee commented on a diff in pull request #10511: Allow empty segmentsTo for segment replacement protocol
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10511:
URL: https://github.com/apache/pinot/pull/10511#discussion_r1154029121
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/StartReplaceSegmentsRequest.java:
##########
@@ -37,11 +37,11 @@ public class StartReplaceSegmentsRequest {
private final List<String> _segmentsTo;
public StartReplaceSegmentsRequest(@JsonProperty("segmentsFrom") @Nullable List<String> segmentsFrom,
- @JsonProperty("segmentsTo") List<String> segmentsTo) {
+ @JsonProperty("segmentsTo") @Nullable List<String> segmentsTo) {
_segmentsFrom = (segmentsFrom == null) ? Collections.emptyList() : segmentsFrom;
- _segmentsTo = segmentsTo;
- Preconditions
- .checkArgument(segmentsTo != null && !segmentsTo.isEmpty(), "'segmentsTo' should not be null or empty");
+ _segmentsTo = (segmentsTo == null) ? Collections.emptyList() : segmentsTo;
+ Preconditions.checkArgument(!_segmentsFrom.isEmpty() || !_segmentsTo.isEmpty(),
+ "Both segmentsFrom and segmentsTo cannot be empty");
Review Comment:
lol this is a side effect of Ai. Git `copilot` recommended the above sentence :P
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/StartReplaceSegmentsRequest.java:
##########
@@ -37,11 +37,11 @@ public class StartReplaceSegmentsRequest {
private final List<String> _segmentsTo;
public StartReplaceSegmentsRequest(@JsonProperty("segmentsFrom") @Nullable List<String> segmentsFrom,
- @JsonProperty("segmentsTo") List<String> segmentsTo) {
+ @JsonProperty("segmentsTo") @Nullable List<String> segmentsTo) {
_segmentsFrom = (segmentsFrom == null) ? Collections.emptyList() : segmentsFrom;
- _segmentsTo = segmentsTo;
- Preconditions
- .checkArgument(segmentsTo != null && !segmentsTo.isEmpty(), "'segmentsTo' should not be null or empty");
+ _segmentsTo = (segmentsTo == null) ? Collections.emptyList() : segmentsTo;
+ Preconditions.checkArgument(!_segmentsFrom.isEmpty() || !_segmentsTo.isEmpty(),
+ "Both segmentsFrom and segmentsTo cannot be empty");
Review Comment:
lol this is a side effect of AI. Git `copilot` recommended the above sentence :P
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee merged pull request #10511: Allow empty segmentsTo for segment replacement protocol
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee merged PR #10511:
URL: https://github.com/apache/pinot/pull/10511
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10511: Allow empty segmentsTo for segment replacement protocol
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10511:
URL: https://github.com/apache/pinot/pull/10511#discussion_r1154021959
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/StartReplaceSegmentsRequest.java:
##########
@@ -37,11 +37,11 @@ public class StartReplaceSegmentsRequest {
private final List<String> _segmentsTo;
public StartReplaceSegmentsRequest(@JsonProperty("segmentsFrom") @Nullable List<String> segmentsFrom,
- @JsonProperty("segmentsTo") List<String> segmentsTo) {
+ @JsonProperty("segmentsTo") @Nullable List<String> segmentsTo) {
_segmentsFrom = (segmentsFrom == null) ? Collections.emptyList() : segmentsFrom;
- _segmentsTo = segmentsTo;
- Preconditions
- .checkArgument(segmentsTo != null && !segmentsTo.isEmpty(), "'segmentsTo' should not be null or empty");
+ _segmentsTo = (segmentsTo == null) ? Collections.emptyList() : segmentsTo;
+ Preconditions.checkArgument(!_segmentsFrom.isEmpty() || !_segmentsTo.isEmpty(),
+ "Both segmentsFrom and segmentsTo cannot be empty");
Review Comment:
(nit) This sentence is a little bit confusing :-P
```suggestion
"'segmentsFrom' and 'segmentsTo' cannot both be empty");
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10511: Allow empty segmentsTo for segment replacement protocol
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10511:
URL: https://github.com/apache/pinot/pull/10511#issuecomment-1491001534
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10511?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10511](https://codecov.io/gh/apache/pinot/pull/10511?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (156601e) into [master](https://codecov.io/gh/apache/pinot/commit/40ac22da06b3f2f7cfc3eb90a0b00afad08e8be8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (40ac22d) will **decrease** coverage by `42.09%`.
> The diff coverage is `50.00%`.
```diff
@@ Coverage Diff @@
## master #10511 +/- ##
=============================================
- Coverage 70.20% 28.11% -42.09%
+ Complexity 6113 58 -6055
=============================================
Files 2090 2065 -25
Lines 112206 111409 -797
Branches 17077 16990 -87
=============================================
- Hits 78778 31328 -47450
- Misses 27876 76985 +49109
+ Partials 5552 3096 -2456
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.64% <50.00%> (+0.02%)` | :arrow_up: |
| integration2 | `24.47% <0.00%> (+0.24%)` | :arrow_up: |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10511?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...restlet/resources/StartReplaceSegmentsRequest.java](https://codecov.io/gh/apache/pinot/pull/10511?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzdGxldC9yZXNvdXJjZXMvU3RhcnRSZXBsYWNlU2VnbWVudHNSZXF1ZXN0LmphdmE=) | `66.66% <50.00%> (+4.16%)` | :arrow_up: |
... and [1442 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10511/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] snleee commented on a diff in pull request #10511: Allow empty segmentsTo for segment replacement protocol
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #10511:
URL: https://github.com/apache/pinot/pull/10511#discussion_r1154007077
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/StartReplaceSegmentsRequest.java:
##########
@@ -37,11 +36,9 @@ public class StartReplaceSegmentsRequest {
private final List<String> _segmentsTo;
public StartReplaceSegmentsRequest(@JsonProperty("segmentsFrom") @Nullable List<String> segmentsFrom,
- @JsonProperty("segmentsTo") List<String> segmentsTo) {
+ @JsonProperty("segmentsTo") @Nullable List<String> segmentsTo) {
_segmentsFrom = (segmentsFrom == null) ? Collections.emptyList() : segmentsFrom;
- _segmentsTo = segmentsTo;
- Preconditions
Review Comment:
done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10511: Allow empty segmentsTo for segment replacement protocol
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10511:
URL: https://github.com/apache/pinot/pull/10511#discussion_r1153857148
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/StartReplaceSegmentsRequest.java:
##########
@@ -37,11 +36,9 @@ public class StartReplaceSegmentsRequest {
private final List<String> _segmentsTo;
public StartReplaceSegmentsRequest(@JsonProperty("segmentsFrom") @Nullable List<String> segmentsFrom,
- @JsonProperty("segmentsTo") List<String> segmentsTo) {
+ @JsonProperty("segmentsTo") @Nullable List<String> segmentsTo) {
_segmentsFrom = (segmentsFrom == null) ? Collections.emptyList() : segmentsFrom;
- _segmentsTo = segmentsTo;
- Preconditions
Review Comment:
(minor) Verify one of them is not empty
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org