You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "dttung2905 (via GitHub)" <gi...@apache.org> on 2024/02/08 23:59:03 UTC
[PR] [DRAFT] Add config option for timezone [pinot]
dttung2905 opened a new pull request, #12386:
URL: https://github.com/apache/pinot/pull/12386
Allow setting custom timezone for invididual components (Controller, Broker, Server) Reference : https://docs.pinot.apache.org/developers/advanced/advanced-pinot-setup#pinot-controller
TODO:
- [ ] Make sure all the existing tests pass
- [ ] Find out which lines in the code base import env var `TZ=UTC`
- [ ] FInd out whether to add new test case or not
- [ ] Docs update
- [ ] Changelog
Fix: https://github.com/apache/pinot/issues/12299
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1500595245
##########
pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java:
##########
@@ -295,6 +296,7 @@ public String getStatusDescription() {
});
LOGGER.info("Pinot minion started");
+ _timezone = System.getProperty("user.timezone");
Review Comment:
If it's only used by test, then I feel you can directly use `System.getProperty("user.timezone");` in the test?
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1956244659
> Can you please also add a test in any integration test for the time zone?
Will definitely do that. Could you point me to the relevant place for integration test ?
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1955302764
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12386?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `5 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`8c395ff`)](https://app.codecov.io/gh/apache/pinot/commit/8c395ff0788099cdb2e36b768e0694853262b942?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.73% compared to head [(`bc3cabb`)](https://app.codecov.io/gh/apache/pinot/pull/12386?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 34.86%.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12386?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ache/pinot/common/utils/ServiceStartableUtils.java](https://app.codecov.io/gh/apache/pinot/pull/12386?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2VydmljZVN0YXJ0YWJsZVV0aWxzLmphdmE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12386?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12386 +/- ##
=============================================
- Coverage 61.73% 34.86% -26.87%
+ Complexity 207 6 -201
=============================================
Files 2436 2360 -76
Lines 133173 129432 -3741
Branches 20631 20073 -558
=============================================
- Hits 82220 45132 -37088
- Misses 44907 81066 +36159
+ Partials 6046 3234 -2812
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.69%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.86% <0.00%> (-26.75%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.84% <0.00%> (-26.86%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.84% <0.00%> (-26.75%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.86% <0.00%> (-26.87%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.71% <0.00%> (-15.03%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.71% <0.00%> (-0.15%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12386/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12386?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: 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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1497275364
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -61,6 +65,7 @@ public static void applyClusterConfig(PinotConfiguration instanceConfig, String
.build();
zkClient.waitUntilConnected(zkClientConnectionTimeoutMs, TimeUnit.MILLISECONDS);
+ setupTimezone(instanceConfig);
Review Comment:
I feel you should move this after line 94, which applies the cluster config from zookeeper.
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1956189662
> @xiangfu0 I parked the config name `pinot.timezone` under `CommonConstants`, not sure its the right place but I think it should be common for all pinot components ( contrller, broker, server, minion ).
>
> Another question I have is how does each component pick up relevant value from config ( for example `pinot.timezone` ). I did some diggings and seems like it points to `readControllerConfigFromFile` and `readControllerConfigFromFile` in org.apache.pinot.tools.utils.PinotConfigUtils`
I feel what you've done which uses cluster config is a better choice here. Since we want this config to apply to all the components.
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1498450893
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -61,6 +65,7 @@ public static void applyClusterConfig(PinotConfiguration instanceConfig, String
.build();
zkClient.waitUntilConnected(zkClientConnectionTimeoutMs, TimeUnit.MILLISECONDS);
+ setupTimezone(instanceConfig);
Review Comment:
Oh one of the reason I put it there is because the method might return prematurely in line 74 if there is nothing found under `CLUSTER_CONFIG_ZK_PATH_TEMPLATE` path. Hence, timezone is not set. Wdyt ?
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1968476338
Thanks. I have updated the PR description to include a release note
--
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
Re: [PR] [DRAFT] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1954128489
Thanks @xiangfu0 for the feedback. I will update it later
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1956356221
> > Can you please also add a test in any integration test for the time zone?
>
> Will definitely do that. Could you point me to the relevant place for integration test ?
In `org.apache.pinot.controller.helix.ControllerTest,` there is `getDefaultControllerConfiguration()`;
In `org.apache.pinot.integration.tests.ClusterTest`, there are methods like `protected PinotConfiguration getBrokerConf(int brokerId)`, `getDefaultServerConfiguration()`, `getDefaultMinionConfiguration()`
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1965369289
I will update the gitbook with a separate PR for docs, not sure where to update for the release note. Could you point me to it?
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1499534130
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -61,6 +65,7 @@ public static void applyClusterConfig(PinotConfiguration instanceConfig, String
.build();
zkClient.waitUntilConnected(zkClientConnectionTimeoutMs, TimeUnit.MILLISECONDS);
+ setupTimezone(instanceConfig);
Review Comment:
right, my feeling here is that if users doesn't set up time zone from cluster config ,then it's pretty hard to manage those configs.
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1961271449
Also please fix the linter.
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1962158063
Only one failed workflow. I suspect its due to the flaky checkout github action runner
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1965074955
Please also add a release note section, mention how to configure it as well as that all the default deployment will use UTC instead of local time zone by default, which is backward incompatible.
If we want to keep backward compatible, then we need to use local time zone as default time zone.
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1965926125
Cool! Please add a section of Release notes in the PR description, so next release manager will just pick it up :)
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1499534130
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -61,6 +65,7 @@ public static void applyClusterConfig(PinotConfiguration instanceConfig, String
.build();
zkClient.waitUntilConnected(zkClientConnectionTimeoutMs, TimeUnit.MILLISECONDS);
+ setupTimezone(instanceConfig);
Review Comment:
right, my feeling here is that if users doesn't set up time zone from cluster config ,then it's pretty hard to manage those configs.
I would like to move those time utils to be closer to users.
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1500589288
##########
pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java:
##########
@@ -295,6 +296,7 @@ public String getStatusDescription() {
});
LOGGER.info("Pinot minion started");
+ _timezone = System.getProperty("user.timezone");
Review Comment:
if there is a conflict between system property and pinot config, what would you prefer?
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1501283722
##########
pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java:
##########
@@ -295,6 +296,7 @@ public String getStatusDescription() {
});
LOGGER.info("Pinot minion started");
+ _timezone = System.getProperty("user.timezone");
Review Comment:
Thank you for the feedback. I will just remove this method entirely
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1499534130
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -61,6 +65,7 @@ public static void applyClusterConfig(PinotConfiguration instanceConfig, String
.build();
zkClient.waitUntilConnected(zkClientConnectionTimeoutMs, TimeUnit.MILLISECONDS);
+ setupTimezone(instanceConfig);
Review Comment:
right, my feeling here is that if users doesn't set up time zone from cluster config ,then it's pretty hard 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
Re: [PR] [DRAFT] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1955243143
@xiangfu0 I parked the config name `pinot.timezone` under `CommonConstants`, not sure its the right place but I think it should be common for all pinot components ( contrller, broker, server, minion ).
Another question I have is how does each component pick up relevant value from config ( for example `pinot.timezone` ). I did some diggings and seems like it points to `readControllerConfigFromFile` and `readControllerConfigFromFile` in org.apache.pinot.tools.utils.PinotConfigUtils`
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1956181796
Can you please also add a test in any integration test for the time zone?
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on code in PR #12386:
URL: https://github.com/apache/pinot/pull/12386#discussion_r1500064984
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/ServiceStartableUtils.java:
##########
@@ -61,6 +65,7 @@ public static void applyClusterConfig(PinotConfiguration instanceConfig, String
.build();
zkClient.waitUntilConnected(zkClientConnectionTimeoutMs, TimeUnit.MILLISECONDS);
+ setupTimezone(instanceConfig);
Review Comment:
Hmm so what do you suggest we do in this case? Should we call `setupTimezone(instanceConfig)` twice: in both line 73 before the `return` and in line 94 at the end of the method
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #12386:
URL: https://github.com/apache/pinot/pull/12386
--
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
Re: [PR] Add config option for timezone [pinot]
Posted by "dttung2905 (via GitHub)" <gi...@apache.org>.
dttung2905 commented on PR #12386:
URL: https://github.com/apache/pinot/pull/12386#issuecomment-1965347684
> Please also add a release note section, mention how to configure it as well as that all the default deployment will use UTC instead of local time zone by default, which is backward incompatible.
>
> If we want to keep backward compatible, then we need to use local time zone as default time zone.
Backward compatibility is a huge plus, and imo we should always support it when we can. I can make changes to the code.
--
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