You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/06/07 22:19:01 UTC
[GitHub] [pinot] timsants opened a new pull request, #8856: BUGFIX: Adding util for getting URL from InstanceConfig
timsants opened a new pull request, #8856:
URL: https://github.com/apache/pinot/pull/8856
Adding a util for getting URL from an InstanceConfig. This is required to fix some places where http is hardcoded which will fail if TLS is enabld.
--
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] xiangfu0 commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r891758211
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/InstanceUtils.java:
##########
@@ -119,4 +122,36 @@ public static void updateHelixInstanceConfig(InstanceConfig instanceConfig, Inst
mapFields.remove(POOL_KEY);
}
}
+
+ /**
+ * Returns an instance admin URL from a given InstanceConfig. Will decipher whether the URL protocol should
+ * be HTTP vs HTTPS.
+ */
+ public static URL getInstanceURL(InstanceConfig instanceConfig) {
Review Comment:
This is for pinot controller only？For pinot broker, you will have one port for query and one port for admin.
--
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] timsants commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r892904111
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,42 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port.
+ */
+ public String getComponentUrl() {
+ String hostName = _proxy.getHostName();
+ // Backward-compatible with legacy hostname of format 'Controller_<hostname>'
Review Comment:
I didn't see a specific example of this. I just noticed that other places that were parsing the hostname was doing the same. E.g. [PinotHelixResourceManager](https://github.com/apache/pinot/blob/master/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java#L221)
--
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] timsants commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r918546748
##########
pinot-controller/src/main/java/org/apache/pinot/controller/tuner/RealTimeAutoIndexTuner.java:
##########
@@ -42,4 +42,10 @@ public TableConfig apply(PinotHelixResourceManager pinotHelixResourceManager,
initialIndexingConfig.setNoDictionaryColumns(schema.getMetricNames());
return tableConfig;
}
+
+ @Override
+ public TableConfig apply(PinotHelixResourceManager pinotHelixResourceManager,
+ TableConfig tableConfig, Schema schema, Map<String, String> extraProperties, Map<String, String> httpHeaders) {
+ return apply(pinotHelixResourceManager, tableConfig, schema, extraProperties);
Review Comment:
Yes its ignored for this case since there are no remote calls being made for this TableConfigTuner impl. Shall we just name the param `ignored` to make it 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: 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] walterddr commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r921352186
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,31 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port. Returns null
+ * if the URL cannot be constructed.
+ */
+ public String getComponentUrl() {
Review Comment:
chatted with @timsants offline and we decided to leave it as is. since the server admin ports are not usually directly interacted by user
--
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] walterddr merged pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #8856:
URL: https://github.com/apache/pinot/pull/8856
--
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] timsants commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r918545812
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,31 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port. Returns null
+ * if the URL cannot be constructed.
+ */
+ public String getComponentUrl() {
Review Comment:
InstanceConfig is a class used by all the Pinot components (broker, server, controller, minion). So this util was supposed to serve the purpose of getting the REST API URL for any of those components. So I can update the method name/javadoc to be clearer.
Thanks for pointing out that the server should route to "adminPort" instead of "port". Is this the case only for calls to server?
--
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] walterddr commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r920595867
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,31 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port. Returns null
+ * if the URL cannot be constructed.
+ */
+ public String getComponentUrl() {
Review Comment:
i would suggest renaming it to getComponengRestResourceUrl() if the goal is to interact with user. and always returns the port for using the REST APIs; (and obviously the secure port when security is enabled)
--
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] timsants commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r897499499
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,42 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port.
+ */
+ public String getComponentUrl() {
+ String hostName = _proxy.getHostName();
+ // Backward-compatible with legacy hostname of format 'Controller_<hostname>'
Review Comment:
Thanks for checking on this Jackie. I went ahead and removed the backward compatible handling.
--
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] walterddr commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r920595325
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,31 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port. Returns null
+ * if the URL cannot be constructed.
+ */
+ public String getComponentUrl() {
Review Comment:
yeah so InstanceConfig host - port is sort of like a "helix" participant concept. where participate registers with helix of its service host/port.
- for server there's admin, netty and grpc port. the main propose of pinot server is to talk to pinot-broker so the netty port is usually registered with helix (as the main service it provides).
- if the idea is to provide a user facing endpoint, then the admin port is the one user will directly interact.
--
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 #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8856:
URL: https://github.com/apache/pinot/pull/8856#issuecomment-1149249449
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8856?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 [#8856](https://codecov.io/gh/apache/pinot/pull/8856?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (39335e0) into [master](https://codecov.io/gh/apache/pinot/commit/d1d2ddbc116f6dddbefdf1e69dd409434eaae0e4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d1d2ddb) will **decrease** coverage by `3.62%`.
> The diff coverage is `14.70%`.
```diff
@@ Coverage Diff @@
## master #8856 +/- ##
============================================
- Coverage 69.80% 66.17% -3.63%
+ Complexity 4659 4482 -177
============================================
Files 1741 1291 -450
Lines 91476 65912 -25564
Branches 13677 10345 -3332
============================================
- Hits 63854 43619 -20235
+ Misses 23199 19180 -4019
+ Partials 4423 3113 -1310
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `66.17% <14.70%> (-0.06%)` | :arrow_down: |
| 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/8856?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/pinot/common/utils/config/InstanceUtils.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvY29uZmlnL0luc3RhbmNlVXRpbHMuamF2YQ==) | `71.87% <0.00%> (-23.96%)` | :arrow_down: |
| [...pinot/common/utils/fetcher/HttpSegmentFetcher.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZmV0Y2hlci9IdHRwU2VnbWVudEZldGNoZXIuamF2YQ==) | `7.69% <0.00%> (-53.85%)` | :arrow_down: |
| [...inot/core/query/executor/sql/SqlQueryExecutor.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9zcWwvU3FsUXVlcnlFeGVjdXRvci5qYXZh) | `0.00% <0.00%> (-64.11%)` | :arrow_down: |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `78.80% <22.22%> (-5.56%)` | :arrow_down: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [695 more](https://codecov.io/gh/apache/pinot/pull/8856/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8856?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8856?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8082838...39335e0](https://codecov.io/gh/apache/pinot/pull/8856?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] timsants commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r891922823
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/InstanceUtils.java:
##########
@@ -119,4 +122,36 @@ public static void updateHelixInstanceConfig(InstanceConfig instanceConfig, Inst
mapFields.remove(POOL_KEY);
}
}
+
+ /**
+ * Returns an instance admin URL from a given InstanceConfig. Will decipher whether the URL protocol should
+ * be HTTP vs HTTPS.
+ */
+ public static URL getInstanceURL(InstanceConfig instanceConfig) {
+ String hostname = instanceConfig.getHostName();
+ if (hostname.startsWith(Helix.PREFIX_OF_SERVER_INSTANCE)) {
Review Comment:
Will create a method that supports all the components, checking for the different prefixes.
--
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 #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r894096750
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,42 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port.
+ */
+ public String getComponentUrl() {
+ String hostName = _proxy.getHostName();
+ // Backward-compatible with legacy hostname of format 'Controller_<hostname>'
Review Comment:
That check is introduced in #5057, and I think we have finished the migration, and this backward compatible handling can be removed
--
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] xiangfu0 commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r891758513
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/InstanceUtils.java:
##########
@@ -119,4 +122,36 @@ public static void updateHelixInstanceConfig(InstanceConfig instanceConfig, Inst
mapFields.remove(POOL_KEY);
}
}
+
+ /**
+ * Returns an instance admin URL from a given InstanceConfig. Will decipher whether the URL protocol should
+ * be HTTP vs HTTPS.
+ */
+ public static URL getInstanceURL(InstanceConfig instanceConfig) {
Review Comment:
Maybe change this method to getInstanceAdminURL
--
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] timsants commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
timsants commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r891922573
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/InstanceUtils.java:
##########
@@ -119,4 +122,36 @@ public static void updateHelixInstanceConfig(InstanceConfig instanceConfig, Inst
mapFields.remove(POOL_KEY);
}
}
+
+ /**
+ * Returns an instance admin URL from a given InstanceConfig. Will decipher whether the URL protocol should
+ * be HTTP vs HTTPS.
+ */
+ public static URL getInstanceURL(InstanceConfig instanceConfig) {
Review Comment:
Going to go the approach of leveraging the ExtraInstanceConfig class.
--
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] walterddr commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r920595867
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,31 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port. Returns null
+ * if the URL cannot be constructed.
+ */
+ public String getComponentUrl() {
Review Comment:
i would suggest renaming it to getComponengRestResourceUrl() if the goal is to interact with user. and always use the REST API.
--
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] walterddr commented on pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #8856:
URL: https://github.com/apache/pinot/pull/8856#issuecomment-1212263059
Unfortunately this is not correct.. HTTPS can be enabled without TLS config.
so I think we should actually determine this based on `ControllerConfig#"controller.broker.protocol"`
--
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 #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r892895428
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,42 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port.
+ */
+ public String getComponentUrl() {
+ String hostName = _proxy.getHostName();
+ // Backward-compatible with legacy hostname of format 'Controller_<hostname>'
Review Comment:
I don't think we allow this legacy hostname format though. Do you see example of this legacy format?
--
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] walterddr commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r916029077
##########
pinot-common/src/main/java/org/apache/pinot/common/helix/ExtraInstanceConfig.java:
##########
@@ -44,4 +45,31 @@ public String getTlsPort() {
public void setTlsPort(String tlsPort) {
_proxy.getRecord().setSimpleField(PinotInstanceConfigProperty.PINOT_TLS_PORT.toString(), tlsPort);
}
+
+ /**
+ * Returns an instance URL from the InstanceConfig. Will set the appropriate protocol and port. Returns null
+ * if the URL cannot be constructed.
+ */
+ public String getComponentUrl() {
Review Comment:
the javadoc is not clear. is getComponentUrl returning the URL (host/port) for the component? if so. which one would it be for broker/server ? for example I think REST API calls to server should route to "adminPort" but not "port".
should we give it a better javadoc / name?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/tuner/RealTimeAutoIndexTuner.java:
##########
@@ -42,4 +42,10 @@ public TableConfig apply(PinotHelixResourceManager pinotHelixResourceManager,
initialIndexingConfig.setNoDictionaryColumns(schema.getMetricNames());
return tableConfig;
}
+
+ @Override
+ public TableConfig apply(PinotHelixResourceManager pinotHelixResourceManager,
+ TableConfig tableConfig, Schema schema, Map<String, String> extraProperties, Map<String, String> httpHeaders) {
+ return apply(pinotHelixResourceManager, tableConfig, schema, extraProperties);
Review Comment:
are we ignoring the httpHeaders here?
--
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] xiangfu0 commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r891794790
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/InstanceUtils.java:
##########
@@ -119,4 +122,36 @@ public static void updateHelixInstanceConfig(InstanceConfig instanceConfig, Inst
mapFields.remove(POOL_KEY);
}
}
+
+ /**
+ * Returns an instance admin URL from a given InstanceConfig. Will decipher whether the URL protocol should
+ * be HTTP vs HTTPS.
+ */
+ public static URL getInstanceURL(InstanceConfig instanceConfig) {
+ String hostname = instanceConfig.getHostName();
+ if (hostname.startsWith(Helix.PREFIX_OF_SERVER_INSTANCE)) {
Review Comment:
hmm, so this is for server prefix?
--
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] xiangfu0 commented on a diff in pull request #8856: BUGFIX: Adding util for getting URL from InstanceConfig
Posted by GitBox <gi...@apache.org>.
xiangfu0 commented on code in PR #8856:
URL: https://github.com/apache/pinot/pull/8856#discussion_r891758211
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/InstanceUtils.java:
##########
@@ -119,4 +122,36 @@ public static void updateHelixInstanceConfig(InstanceConfig instanceConfig, Inst
mapFields.remove(POOL_KEY);
}
}
+
+ /**
+ * Returns an instance admin URL from a given InstanceConfig. Will decipher whether the URL protocol should
+ * be HTTP vs HTTPS.
+ */
+ public static URL getInstanceURL(InstanceConfig instanceConfig) {
Review Comment:
This is for pinot controller only, for pinot broker, you will have one port for query and one port for admin.
--
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