You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ArbaazKhan1 (via GitHub)" <gi...@apache.org> on 2024/06/05 20:10:42 UTC
[PR] Timeout value addd to wait for scans servers [accumulo]
ArbaazKhan1 opened a new pull request, #4638:
URL: https://github.com/apache/accumulo/pull/4638
Closes [issue#4532](https://github.com/apache/accumulo/issues/4532)
Changed `enableTabletServerFallback` to `timeToWaitForScanServers` and made it a timeout value. Please take a look at the class description and see if the explanation `timeToWaitForScanServers` is sufficient enough. This update will need to be added into 2.1 as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1636869070
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,25 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
+ * how long to wait for scan servers to load before falling back to tablet servers is desired.
+ * Falling back to tablet servers may cause tablets to be loaded that are not currently loaded. When
+ * this setting is given a wait time and there are no scan servers, it will wait for scan servers to
+ * be available. This setting avoids loading tablets on tablet servers when scans servers are
+ * temporarily unavailable which could be caused by normal cluster activity. You can specify the
+ * wait time using different units to precisely control the wait duration. The supported units are:
+ * <ul>
+ * <li>"d" for days</li>
+ * <li>"h" for hours</li>
+ * <li>"m" for minutes</li>
+ * <li>"s" for seconds</li>
+ * <li>"ms" for milliseconds</li>
+ * </ul>
+ * If duration is not specified this setting defaults to 0s, and will disable the wait for scan
+ * servers. When set to a large value, the system will effectively wait for scan servers to become
+ * available before falling back to tablet servers. To ensure the scan servers does not fall back to
+ * tablet servers and continuously waits for scan servers a wait time of 10000d should be
Review Comment:
```suggestion
* available before falling back to tablet servers. To ensure the selector never falls back to scanning
* tablet servers a unrealistic wait time can be set. For instance, 10000d should be
```
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,25 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
+ * how long to wait for scan servers to load before falling back to tablet servers is desired.
+ * Falling back to tablet servers may cause tablets to be loaded that are not currently loaded. When
+ * this setting is given a wait time and there are no scan servers, it will wait for scan servers to
+ * be available. This setting avoids loading tablets on tablet servers when scans servers are
+ * temporarily unavailable which could be caused by normal cluster activity. You can specify the
+ * wait time using different units to precisely control the wait duration. The supported units are:
+ * <ul>
+ * <li>"d" for days</li>
+ * <li>"h" for hours</li>
+ * <li>"m" for minutes</li>
+ * <li>"s" for seconds</li>
+ * <li>"ms" for milliseconds</li>
+ * </ul>
+ * If duration is not specified this setting defaults to 0s, and will disable the wait for scan
+ * servers. When set to a large value, the system will effectively wait for scan servers to become
+ * available before falling back to tablet servers. To ensure the scan servers does not fall back to
+ * tablet servers and continuously waits for scan servers a wait time of 10000d should be
Review Comment:
The client using the selector is what would fall back to using the tablet servers, not the scan servers themselves.
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,25 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
+ * how long to wait for scan servers to load before falling back to tablet servers is desired.
+ * Falling back to tablet servers may cause tablets to be loaded that are not currently loaded. When
+ * this setting is given a wait time and there are no scan servers, it will wait for scan servers to
+ * be available. This setting avoids loading tablets on tablet servers when scans servers are
+ * temporarily unavailable which could be caused by normal cluster activity. You can specify the
+ * wait time using different units to precisely control the wait duration. The supported units are:
+ * <ul>
+ * <li>"d" for days</li>
+ * <li>"h" for hours</li>
+ * <li>"m" for minutes</li>
+ * <li>"s" for seconds</li>
+ * <li>"ms" for milliseconds</li>
+ * </ul>
+ * If duration is not specified this setting defaults to 0s, and will disable the wait for scan
+ * servers. When set to a large value, the system will effectively wait for scan servers to become
Review Comment:
The word "system" is ambiguous. I would recommend using either "client" or "selector" to denote that this logic is handled by the client attempting the scan.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1630263342
##########
core/src/test/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelectorTest.java:
##########
@@ -490,7 +490,7 @@ public void testWaitForScanServers() {
// will wait for scan servers
String defaultProfile =
- "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'enableTabletServerFallback':false,"
+ "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'timeToWaitForScanServers':120ms,"
Review Comment:
Would be good to make `0` value disable it and document that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1640359454
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,25 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
+ * how long to wait for scan servers to load before falling back to tablet servers is desired.
Review Comment:
```suggestion
* how long to wait for scan servers to become available before falling back to tablet servers.
```
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,25 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
+ * how long to wait for scan servers to load before falling back to tablet servers is desired.
+ * Falling back to tablet servers may cause tablets to be loaded that are not currently loaded. When
+ * this setting is given a wait time and there are no scan servers, it will wait for scan servers to
+ * be available. This setting avoids loading tablets on tablet servers when scans servers are
+ * temporarily unavailable which could be caused by normal cluster activity. You can specify the
+ * wait time using different units to precisely control the wait duration. The supported units are:
+ * <ul>
+ * <li>"d" for days</li>
+ * <li>"h" for hours</li>
+ * <li>"m" for minutes</li>
+ * <li>"s" for seconds</li>
+ * <li>"ms" for milliseconds</li>
+ * </ul>
+ * If duration is not specified this setting defaults to 0s, and will disable the wait for scan
+ * servers. When set to a large value, the selector will effectively wait for scan servers to become
Review Comment:
```suggestion
* servers and will fall back to tablet servers immediately. When set to a large value, the selector will effectively wait for scan servers to become
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1630256887
##########
core/src/test/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelectorTest.java:
##########
@@ -490,7 +490,7 @@ public void testWaitForScanServers() {
// will wait for scan servers
String defaultProfile =
- "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'enableTabletServerFallback':false,"
+ "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'timeToWaitForScanServers':120ms,"
Review Comment:
The values do not have to numeric because we parse them. We could accept a timeout value of `never`, if its documented, and in the impl set that to max long.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1636417143
##########
core/src/test/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelectorTest.java:
##########
@@ -490,7 +490,7 @@ public void testWaitForScanServers() {
// will wait for scan servers
String defaultProfile =
- "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'enableTabletServerFallback':false,"
+ "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'timeToWaitForScanServers':120s,"
Review Comment:
Does `120s` need to be surrounded in single quotes?
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -344,16 +366,14 @@ public ScanServerSelections selectServers(ScanServerSelector.SelectorParameters
List<String> orderedScanServers =
orderedScanServersSupplier.get().getOrDefault(profile.group, List.of());
+ Duration scanServerWaitTIme = Duration.ofMillis(profile.getTimeToWaitForScanServers());
Review Comment:
```suggestion
Duration scanServerWaitTime = Duration.ofMillis(profile.getTimeToWaitForScanServers());
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#issuecomment-2168776687
I think this is good to merge, just a few minor comments on the javadoc.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1630077633
##########
core/src/test/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelectorTest.java:
##########
@@ -490,7 +490,7 @@ public void testWaitForScanServers() {
// will wait for scan servers
String defaultProfile =
- "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'enableTabletServerFallback':false,"
+ "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'timeToWaitForScanServers':120ms,"
Review Comment:
What value is used to disable this? Is it `0` ? I'm thinking that this should be documented (how to disable). Also, I think the default value should be whatever it needs to be to disable this as I think that keeps the behavior the same.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1630259893
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,14 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
Review Comment:
The values do not have to numeric because we parse them. We could accept a timeout value of never, if its documented, and in the impl set that to max long. I made this comment on the wrong thread a bit ago, duplicating it 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "ArbaazKhan1 (via GitHub)" <gi...@apache.org>.
ArbaazKhan1 commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1631269201
##########
core/src/test/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelectorTest.java:
##########
@@ -490,7 +490,7 @@ public void testWaitForScanServers() {
// will wait for scan servers
String defaultProfile =
- "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'enableTabletServerFallback':false,"
+ "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'timeToWaitForScanServers':120ms,"
Review Comment:
I agree with having `0` be the disable value. I think with setting duration keeping things consistent with numerical values is best. I'll add a note in the docs about `0` being the default and disable value.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1635527271
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,25 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
+ * how long to wait for scan servers to load before falling back to tablet servers is desired.
+ * Falling back to tablet servers may cause tablets to be loaded that are not currently loaded. When
+ * this setting is given a wait time and there are no scan servers, it will wait for scan servers to
+ * be available. This setting avoids loading tablets on tablet servers when scans servers are
+ * temporarily unavailable which could be caused by normal cluster activity. You can specify the
+ * wait time using different units to precisely control the wait duration. The supported units are:
+ * <ul>
+ * <li>"d" for days</li>
+ * <li>"h" for hours</li>
+ * <li>"m" for minutes</li>
+ * <li>"s" for seconds</li>
+ * <li>"ms" for milliseconds</li>
+ * </ul>
+ * If duration is not specified this setting defaults to 0s, and will disable the wait for scan
+ * servers. When set to a large value, the system will effectively wait for scan servers to become
+ * scan servers to be available before falling back to tablet servers. To ensure the scan servers
Review Comment:
```suggestion
* servers. When set to a large value, the system will effectively wait for scan servers to become
* available before falling back to tablet servers. To ensure the scan servers
```
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -237,11 +250,13 @@ protected static class Profile {
int busyTimeoutMultiplier;
String maxBusyTimeout;
String group = ScanServerSelector.DEFAULT_SCAN_SERVER_GROUP_NAME;
- boolean enableTabletServerFallback = true;
+ String timeToWaitForScanServers = "0s";
transient boolean parsed = false;
transient long parsedMaxBusyTimeout;
+ transient long parsedTimeToWaitForScanServers;
Review Comment:
Could change this to a duration and compute the Duration once in parse. This avoids allocating a Duration object every time its used.
```suggestion
transient Duration parsedTimeToWaitForScanServers;
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1630259893
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,14 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
Review Comment:
The values do not have to numeric because we parse them. We could accept a timeout value of `never`, if its documented, and in the impl set that to max long. I made this comment on the wrong thread a bit ago, duplicating it 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #4638:
URL: https://github.com/apache/accumulo/pull/4638
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1628543868
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -124,7 +125,7 @@
* "maxBusyTimeout":"20m",
* "busyTimeoutMultiplier":8,
* "group":"lowcost",
- * "enableTabletServerFallback":false,
+ * "timeToWaitForScanServers": "120ms,
Review Comment:
120ms seems low to me for an example, but this is subjective feel free to ignore this comment.
```suggestion
* "timeToWaitForScanServers": "120s,
```
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -83,13 +83,14 @@
* certain scans. Second groups can be used to have different hardware/VM types for scans, for
* example could have some scans use expensive high memory VMs and others use cheaper burstable
* VMs.</li>
- * <li><b>enableTabletServerFallback : </b> When there are no scans servers, this setting determines
- * if fallback to tablet servers is desired. Falling back to tablet servers may cause tablets to be
- * loaded that are not currently loaded. When this setting is false and there are no scan servers,
- * it will wait for scan servers to be available. This setting avoids loading tablets on tablet
- * servers when scans servers are temporarily unavailable which could be caused by normal cluster
- * activity. If not specified this setting defaults to true. Set to false to avoid tablet server
- * fallback. Waiting for scan servers is done via
+ * <li><b>timeToWaitForScanServers : </b> When there are no scans servers, this setting determines
Review Comment:
Would be good to work in a suggested value for the case when someone wants to practiacally never fallback. Could use `10000d` for this as its 27 years. Would also be good to add something about the supported time units for this value to the docs. Looking at ConfigurationTypeHelper it supports ms,s,m,h,d units.
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -345,15 +346,13 @@ public ScanServerSelections selectServers(ScanServerSelector.SelectorParameters
orderedScanServersSupplier.get().getOrDefault(profile.group, List.of());
var finalProfile = profile;
- if (orderedScanServers.isEmpty() && !profile.enableTabletServerFallback) {
+ if (orderedScanServers.isEmpty() && profile.timeToWaitForScanServers != null) {
Review Comment:
This is an important config, I don't think we should ignore it if its absent. Think it would be better if the init method failed w/ a clear message if its absent. This will force the user to always set it and make a decision about it.
When backporting to 2.1 may need to ignore if absent, but in 3.1 and elasticity could fail if absent.
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java:
##########
@@ -345,15 +346,13 @@ public ScanServerSelections selectServers(ScanServerSelector.SelectorParameters
orderedScanServersSupplier.get().getOrDefault(profile.group, List.of());
var finalProfile = profile;
- if (orderedScanServers.isEmpty() && !profile.enableTabletServerFallback) {
+ if (orderedScanServers.isEmpty() && profile.timeToWaitForScanServers != null) {
// Wait for scan servers in the configured group to be present.
- orderedScanServers =
- params
- .waitUntil(
- () -> Optional
- .ofNullable(orderedScanServersSupplier.get().get(finalProfile.group)),
- Duration.ofMillis(Long.MAX_VALUE), "scan servers in group : " + profile.group)
- .orElseThrow();
+ orderedScanServers = params.waitUntil(
+ () -> Optional.ofNullable(orderedScanServersSupplier.get().get(finalProfile.group)),
+ Duration
Review Comment:
This config should always be the same, so we could add something to following code to create a duration object once.
https://github.com/apache/accumulo/blob/9ec008706927533adcd95a8e1ded43735836cdab/core/src/main/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelector.java#L236-L242
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#issuecomment-2175907047
@ArbaazKhan1 - if you can merge address the latest comments, then I should be able to merge this in.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Timeout value addd to wait for scans servers [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4638:
URL: https://github.com/apache/accumulo/pull/4638#discussion_r1630256887
##########
core/src/test/java/org/apache/accumulo/core/spi/scan/ConfigurableScanServerSelectorTest.java:
##########
@@ -490,7 +490,7 @@ public void testWaitForScanServers() {
// will wait for scan servers
String defaultProfile =
- "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'enableTabletServerFallback':false,"
+ "{'isDefault':true,'maxBusyTimeout':'5m','busyTimeoutMultiplier':4,'timeToWaitForScanServers':120ms,"
Review Comment:
The values do not have to numeric because we parse them. We could accept a timeout value of `never`, if its documented, and in the impl set that to max long.
Ignore this comment here, meant to make it on other thread.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org