You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "INNOCENT-BOY (via GitHub)" <gi...@apache.org> on 2023/05/12 16:25:31 UTC
[GitHub] [pinot] INNOCENT-BOY opened a new pull request, #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
INNOCENT-BOY opened a new pull request, #10759:
URL: https://github.com/apache/pinot/pull/10759
When we tried to add segmentAssignmentConfigMap to tableConfig, we found that property segmentAssignmentStrategy can't be parsed. After debugging, we found the key ''segmentAssignmentStrategy'' will be serialized to "assignmentStrategy", that caused we can't deserialized this key. So i add @JaonProperty to _assignmentStrategy to keep the same name during serialized and deserialized.
After applying this patch:
<img width="650" alt="image" src="https://github.com/apache/pinot/assets/38196564/5f3f05e1-e383-4961-84c0-02c941a9313c">
Hi @Jackie-Jiang @gortiz @zhtaoxiang, please help me to review this PR.
--
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] INNOCENT-BOY commented on pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on PR #10759:
URL: https://github.com/apache/pinot/pull/10759#issuecomment-1550568118
I think this is a very important Bugfix if you want to use separate segmentAssignStrategy for completed segment of Realtime Table. Because we can't turn on this feature if we won't merge this bugfix. @Jackie-Jiang @gortiz @zhtaoxiang @xiangfu0 @GSharayu
--
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] INNOCENT-BOY commented on pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on PR #10759:
URL: https://github.com/apache/pinot/pull/10759#issuecomment-1554126121
Hi Team, please help me to merge this PR?
--
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 #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10759:
URL: https://github.com/apache/pinot/pull/10759#issuecomment-1546046017
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10759?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 [#10759](https://app.codecov.io/gh/apache/pinot/pull/10759?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (34f7ca2) into [master](https://app.codecov.io/gh/apache/pinot/commit/78c05f6301e4d9a537de8e537ed2b0236400d353?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (78c05f6) will **decrease** coverage by `50.83%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #10759 +/- ##
=============================================
- Coverage 64.56% 13.73% -50.83%
+ Complexity 6449 439 -6010
=============================================
Files 2099 2099
Lines 112928 112928
Branches 17075 17075
=============================================
- Hits 72910 15510 -57400
- Misses 34803 96150 +61347
+ Partials 5215 1268 -3947
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `13.73% <ø> (+<0.01%)` | :arrow_up: |
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://app.codecov.io/gh/apache/pinot/pull/10759?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...nfig/table/assignment/SegmentAssignmentConfig.java](https://app.codecov.io/gh/apache/pinot/pull/10759?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL2Fzc2lnbm1lbnQvU2VnbWVudEFzc2lnbm1lbnRDb25maWcuamF2YQ==) | `0.00% <ø> (ø)` | |
... and [1447 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10759/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] INNOCENT-BOY commented on a diff in pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10759:
URL: https://github.com/apache/pinot/pull/10759#discussion_r1196363274
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/SegmentAssignmentConfig.java:
##########
@@ -34,6 +34,7 @@ public SegmentAssignmentConfig(@JsonProperty(value = "segmentAssignmentStrategy"
_assignmentStrategy = assignmentStrategy;
}
+ @JsonProperty("segmentAssignmentStrategy")
Review Comment:
@gortiz Thanks for your advice，I will try to change codes.
--
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] INNOCENT-BOY commented on a diff in pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10759:
URL: https://github.com/apache/pinot/pull/10759#discussion_r1196363274
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/SegmentAssignmentConfig.java:
##########
@@ -34,6 +34,7 @@ public SegmentAssignmentConfig(@JsonProperty(value = "segmentAssignmentStrategy"
_assignmentStrategy = assignmentStrategy;
}
+ @JsonProperty("segmentAssignmentStrategy")
Review Comment:
@gortiz Thanks for your advice，I will try to
--
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] gortiz commented on a diff in pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10759:
URL: https://github.com/apache/pinot/pull/10759#discussion_r1196344690
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/SegmentAssignmentConfig.java:
##########
@@ -34,6 +34,7 @@ public SegmentAssignmentConfig(@JsonProperty(value = "segmentAssignmentStrategy"
_assignmentStrategy = assignmentStrategy;
}
+ @JsonProperty("segmentAssignmentStrategy")
Review Comment:
You can place `@JsonProperty` in the attribute. Alternatively, you can move the `@JsonPropertyDescription` from the attribute to the getter.
`@JsonProperty` can be placed in attributes, getters and setters (well, and attributes in `@JsonCreator`, but that the meaning there is different). In case an annotation is found in several places, the preference will be getter > setter > attribute. It doesn't mean that we should always place them in the getter, but it means that we should not place annotations in different places in order to make it more difficult to make mistakes.
My personal taste is to place them in attributes (as they are in the front of the class) but I'm not going to block a PR that adds them in the getter (especially if that is what we do in the rest of the code). But I really think that having annotations in attributes and in getters is error prone.
--
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] KKcorps commented on pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #10759:
URL: https://github.com/apache/pinot/pull/10759#issuecomment-1554177933
Thank you for the contribution!
Waiting for the checks to complete. Will merge as soon as they are 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] gortiz commented on a diff in pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10759:
URL: https://github.com/apache/pinot/pull/10759#discussion_r1196145111
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/SegmentAssignmentConfig.java:
##########
@@ -34,6 +34,7 @@ public SegmentAssignmentConfig(@JsonProperty(value = "segmentAssignmentStrategy"
_assignmentStrategy = assignmentStrategy;
}
+ @JsonProperty("segmentAssignmentStrategy")
Review Comment:
I would place the annotation in the attribute, given that there is already a json annotation there
--
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] KKcorps merged pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps merged PR #10759:
URL: https://github.com/apache/pinot/pull/10759
--
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] GSharayu commented on pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "GSharayu (via GitHub)" <gi...@apache.org>.
GSharayu commented on PR #10759:
URL: https://github.com/apache/pinot/pull/10759#issuecomment-1551708417
@snleee Can you also please take a look at this PR
--
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] INNOCENT-BOY commented on a diff in pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10759:
URL: https://github.com/apache/pinot/pull/10759#discussion_r1196314940
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/SegmentAssignmentConfig.java:
##########
@@ -34,6 +34,7 @@ public SegmentAssignmentConfig(@JsonProperty(value = "segmentAssignmentStrategy"
_assignmentStrategy = assignmentStrategy;
}
+ @JsonProperty("segmentAssignmentStrategy")
Review Comment:
Hi @gortiz , thank for your reply.
In my opinion, the first @JsonProperty is used for deserialization and this annotation is needed when you use @JsonCreator on constructor. but the second @JsonProperty is used to serialized.
We can't put only one annotation on the attribute, because @JsonCreator need to together with annotation @JsonProperty.
Last, this way is same like other json class, such as TableConfig (org.apache.pinot.spi.config.table.TableConfig)
--
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] INNOCENT-BOY commented on a diff in pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10759:
URL: https://github.com/apache/pinot/pull/10759#discussion_r1196363274
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/SegmentAssignmentConfig.java:
##########
@@ -34,6 +34,7 @@ public SegmentAssignmentConfig(@JsonProperty(value = "segmentAssignmentStrategy"
_assignmentStrategy = assignmentStrategy;
}
+ @JsonProperty("segmentAssignmentStrategy")
Review Comment:
@gortiz Thanks for your advice，I have changed the code towards correct direction. Please to review again.
--
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] INNOCENT-BOY commented on a diff in pull request #10759: bug fix: add @JsonProperty for variable _assignmentStrategy in SegmentAssignmentConfig
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10759:
URL: https://github.com/apache/pinot/pull/10759#discussion_r1196314940
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/SegmentAssignmentConfig.java:
##########
@@ -34,6 +34,7 @@ public SegmentAssignmentConfig(@JsonProperty(value = "segmentAssignmentStrategy"
_assignmentStrategy = assignmentStrategy;
}
+ @JsonProperty("segmentAssignmentStrategy")
Review Comment:
Hi @gortiz , thank for your reply.
In my opinion, the first @JsonProperty is used for deserialization and this annotation is needed when you use @JsonCreator on constructor. but the second @JsonProperty is used to serialized.
We can't put only one annotation on the attribute, because @JsonCreator need to together with annotation @JsonProperty.
Last, this way refers to other json class, such as TableConfig (org.apache.pinot.spi.config.table.TableConfig)
--
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