You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/04/21 22:13:34 UTC
[GitHub] [druid] maytasm opened a new pull request #11144: Make sure updating coordinator config is protected against race condition
maytasm opened a new pull request #11144:
URL: https://github.com/apache/druid/pull/11144
Make sure updating coordinator config is protected against race condition
### Description
Many coordinator config bundle multiple configurations into the same key. For example, the auto compaction config (`coordinator.compaction.config`) contains all the compaction configs for multiple datasources within the same payload. In many places, when we do updates on coordinator config, we first do a read to get the current value and apply the change. The read and write does not have any lock to prevent concurrent update.
For example, let’s say we currently have current value for compaction config: {datasource x} and we have concurrent deleteCompactionConfig and addOrUpdateCompactionConfig calls happening at the same time in the following order:
deleteCompactionConfig call to delete x -> deleteCompactionConfig read current which is {x} from JacksonConfigManager-> addOrUpdateCompactionConfig calls to add y -> addOrUpdateCompactionConfig got current which is {x} from JacksonConfigManager-> deleteCompactionConfig write {} to database -> addOrUpdateCompactionConfig write {x, y} to database. Final result is compaction config for x and y in the database. This is incorrect as both calls (call to deleteCompactionConfig and addOrUpdateCompactionConfig) would have returned success and user would expect x to be deleted.
This PR changes the update to `coordinator.compaction.config`, `coordinator.config` and `lookupsConfig` to pass the current config value (which is used to build the new config value) to the MetadataStorageConnector and to use MetadataStorageConnector.compareAndSwap method instead. The API to update these configs may now fail if between the time we retrieve the current config value to the time the database update happened, the config value changed.
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11144:
URL: https://github.com/apache/druid/pull/11144#discussion_r629635984
##########
File path: core/src/main/java/org/apache/druid/common/config/ConfigManager.java
##########
@@ -168,64 +171,99 @@ private void poll()
return holder.getReference();
}
-
public <T> SetResult set(final String key, final ConfigSerde<T> serde, final T obj)
{
- if (obj == null || !started) {
- if (obj == null) {
- return SetResult.fail(new IllegalAccessException("input obj is null"));
+ return set(key, serde, null, obj);
+ }
+
+ public <T> SetResult set(final String key, final ConfigSerde<T> serde, @Nullable final T oldObject, final T newObject)
+ {
+ if (newObject == null || !started) {
+ if (newObject == null) {
+ return SetResult.fail(new IllegalAccessException("input obj is null"), false);
Review comment:
Fixed
##########
File path: server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
##########
@@ -162,27 +165,68 @@ public Response deleteCompactionConfig(
@Context HttpServletRequest req
)
{
- final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
- final Map<String, DataSourceCompactionConfig> configs = current
- .getCompactionConfigs()
- .stream()
- .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+ Callable<SetResult> callable = () -> {
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
+ final Map<String, DataSourceCompactionConfig> configs = current
+ .getCompactionConfigs()
+ .stream()
+ .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+
+ final DataSourceCompactionConfig config = configs.remove(dataSource);
+ if (config == null) {
+ return SetResult.fail(new NoSuchElementException("datasource not found"), false);
+ }
+
+ return manager.set(
+ CoordinatorCompactionConfig.CONFIG_KEY,
+ // Do database insert without swap if the current config is empty as this means the config may be null in the database
+ CoordinatorCompactionConfig.empty().equals(current) ? null : current,
+ CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(configs.values())),
+ new AuditInfo(author, comment, req.getRemoteAddr())
+ );
+ };
+ return updateConfigHelper(callable);
+ }
- final DataSourceCompactionConfig config = configs.remove(dataSource);
- if (config == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
+ @VisibleForTesting
+ Response updateConfigHelper(Callable<SetResult> updateMethod)
+ {
+ int attemps = 0;
+ SetResult setResult = null;
+ try {
+ while (attemps < UPDATE_NUM_RETRY) {
+ setResult = updateMethod.call();
+ if (setResult.isOk() || !setResult.isRetryable()) {
+ break;
+ }
+ attemps++;
+ updateRetryDelay();
+ }
+ }
Review comment:
I forgot about RetryUtils. Fixed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11144:
URL: https://github.com/apache/druid/pull/11144#discussion_r629678850
##########
File path: server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
##########
@@ -162,27 +165,68 @@ public Response deleteCompactionConfig(
@Context HttpServletRequest req
)
{
- final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
- final Map<String, DataSourceCompactionConfig> configs = current
- .getCompactionConfigs()
- .stream()
- .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+ Callable<SetResult> callable = () -> {
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
+ final Map<String, DataSourceCompactionConfig> configs = current
+ .getCompactionConfigs()
+ .stream()
+ .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+
+ final DataSourceCompactionConfig config = configs.remove(dataSource);
+ if (config == null) {
+ return SetResult.fail(new NoSuchElementException("datasource not found"), false);
+ }
+
+ return manager.set(
+ CoordinatorCompactionConfig.CONFIG_KEY,
+ // Do database insert without swap if the current config is empty as this means the config may be null in the database
+ CoordinatorCompactionConfig.empty().equals(current) ? null : current,
+ CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(configs.values())),
+ new AuditInfo(author, comment, req.getRemoteAddr())
+ );
+ };
+ return updateConfigHelper(callable);
+ }
- final DataSourceCompactionConfig config = configs.remove(dataSource);
- if (config == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
+ @VisibleForTesting
+ Response updateConfigHelper(Callable<SetResult> updateMethod)
+ {
+ int attemps = 0;
+ SetResult setResult = null;
+ try {
+ while (attemps < UPDATE_NUM_RETRY) {
+ setResult = updateMethod.call();
+ if (setResult.isOk() || !setResult.isRetryable()) {
+ break;
+ }
+ attemps++;
+ updateRetryDelay();
+ }
+ }
Review comment:
I made the issue at https://github.com/apache/druid/issues/11230
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11144:
URL: https://github.com/apache/druid/pull/11144#discussion_r629635984
##########
File path: core/src/main/java/org/apache/druid/common/config/ConfigManager.java
##########
@@ -168,64 +171,99 @@ private void poll()
return holder.getReference();
}
-
public <T> SetResult set(final String key, final ConfigSerde<T> serde, final T obj)
{
- if (obj == null || !started) {
- if (obj == null) {
- return SetResult.fail(new IllegalAccessException("input obj is null"));
+ return set(key, serde, null, obj);
+ }
+
+ public <T> SetResult set(final String key, final ConfigSerde<T> serde, @Nullable final T oldObject, final T newObject)
+ {
+ if (newObject == null || !started) {
+ if (newObject == null) {
+ return SetResult.fail(new IllegalAccessException("input obj is null"), false);
Review comment:
Fixed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11144:
URL: https://github.com/apache/druid/pull/11144#discussion_r629660293
##########
File path: server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
##########
@@ -162,27 +165,68 @@ public Response deleteCompactionConfig(
@Context HttpServletRequest req
)
{
- final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
- final Map<String, DataSourceCompactionConfig> configs = current
- .getCompactionConfigs()
- .stream()
- .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+ Callable<SetResult> callable = () -> {
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
+ final Map<String, DataSourceCompactionConfig> configs = current
+ .getCompactionConfigs()
+ .stream()
+ .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+
+ final DataSourceCompactionConfig config = configs.remove(dataSource);
+ if (config == null) {
+ return SetResult.fail(new NoSuchElementException("datasource not found"), false);
+ }
+
+ return manager.set(
+ CoordinatorCompactionConfig.CONFIG_KEY,
+ // Do database insert without swap if the current config is empty as this means the config may be null in the database
+ CoordinatorCompactionConfig.empty().equals(current) ? null : current,
+ CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(configs.values())),
+ new AuditInfo(author, comment, req.getRemoteAddr())
+ );
+ };
+ return updateConfigHelper(callable);
+ }
- final DataSourceCompactionConfig config = configs.remove(dataSource);
- if (config == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
+ @VisibleForTesting
+ Response updateConfigHelper(Callable<SetResult> updateMethod)
+ {
+ int attemps = 0;
+ SetResult setResult = null;
+ try {
+ while (attemps < UPDATE_NUM_RETRY) {
+ setResult = updateMethod.call();
+ if (setResult.isOk() || !setResult.isRetryable()) {
+ break;
+ }
+ attemps++;
+ updateRetryDelay();
+ }
+ }
Review comment:
Ah cool, I guess it isn't as configurable as what I thought. Do you think it would be worth maybe making an issue to suggest making it more configurable to not use hard-coded stuffs and link to these places as potential areas that could benefit from such a refactor?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11144:
URL: https://github.com/apache/druid/pull/11144#discussion_r629623614
##########
File path: core/src/main/java/org/apache/druid/common/config/ConfigManager.java
##########
@@ -168,64 +171,99 @@ private void poll()
return holder.getReference();
}
-
public <T> SetResult set(final String key, final ConfigSerde<T> serde, final T obj)
{
- if (obj == null || !started) {
- if (obj == null) {
- return SetResult.fail(new IllegalAccessException("input obj is null"));
+ return set(key, serde, null, obj);
+ }
+
+ public <T> SetResult set(final String key, final ConfigSerde<T> serde, @Nullable final T oldObject, final T newObject)
+ {
+ if (newObject == null || !started) {
+ if (newObject == null) {
+ return SetResult.fail(new IllegalAccessException("input obj is null"), false);
Review comment:
super nitpick: i know this isn't new or your fault, but "obj" doesn't seem to need to be shortened
##########
File path: server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
##########
@@ -162,27 +165,68 @@ public Response deleteCompactionConfig(
@Context HttpServletRequest req
)
{
- final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
- final Map<String, DataSourceCompactionConfig> configs = current
- .getCompactionConfigs()
- .stream()
- .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+ Callable<SetResult> callable = () -> {
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
+ final Map<String, DataSourceCompactionConfig> configs = current
+ .getCompactionConfigs()
+ .stream()
+ .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+
+ final DataSourceCompactionConfig config = configs.remove(dataSource);
+ if (config == null) {
+ return SetResult.fail(new NoSuchElementException("datasource not found"), false);
+ }
+
+ return manager.set(
+ CoordinatorCompactionConfig.CONFIG_KEY,
+ // Do database insert without swap if the current config is empty as this means the config may be null in the database
+ CoordinatorCompactionConfig.empty().equals(current) ? null : current,
+ CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(configs.values())),
+ new AuditInfo(author, comment, req.getRemoteAddr())
+ );
+ };
+ return updateConfigHelper(callable);
+ }
- final DataSourceCompactionConfig config = configs.remove(dataSource);
- if (config == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
+ @VisibleForTesting
+ Response updateConfigHelper(Callable<SetResult> updateMethod)
+ {
+ int attemps = 0;
+ SetResult setResult = null;
+ try {
+ while (attemps < UPDATE_NUM_RETRY) {
+ setResult = updateMethod.call();
+ if (setResult.isOk() || !setResult.isRetryable()) {
+ break;
+ }
+ attemps++;
+ updateRetryDelay();
+ }
+ }
Review comment:
Does RetryUtils not suitable for some reason?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] lgtm-com[bot] commented on pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11144:
URL: https://github.com/apache/druid/pull/11144#issuecomment-833140324
This pull request **introduces 1 alert** when merging 5253b5318b3ef9cb0398991a36609916e7dfb1b0 into 34169c8550dbe0a3fc9043aa21d221b9c7502039 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-294f23a40e66f81d0ea79c5e2bfd211185aae8c5)
**new alerts:**
* 1 for Dereferenced variable may be null
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11144:
URL: https://github.com/apache/druid/pull/11144#discussion_r629647863
##########
File path: server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
##########
@@ -162,27 +165,68 @@ public Response deleteCompactionConfig(
@Context HttpServletRequest req
)
{
- final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
- final Map<String, DataSourceCompactionConfig> configs = current
- .getCompactionConfigs()
- .stream()
- .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+ Callable<SetResult> callable = () -> {
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
+ final Map<String, DataSourceCompactionConfig> configs = current
+ .getCompactionConfigs()
+ .stream()
+ .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+
+ final DataSourceCompactionConfig config = configs.remove(dataSource);
+ if (config == null) {
+ return SetResult.fail(new NoSuchElementException("datasource not found"), false);
+ }
+
+ return manager.set(
+ CoordinatorCompactionConfig.CONFIG_KEY,
+ // Do database insert without swap if the current config is empty as this means the config may be null in the database
+ CoordinatorCompactionConfig.empty().equals(current) ? null : current,
+ CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(configs.values())),
+ new AuditInfo(author, comment, req.getRemoteAddr())
+ );
+ };
+ return updateConfigHelper(callable);
+ }
- final DataSourceCompactionConfig config = configs.remove(dataSource);
- if (config == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
+ @VisibleForTesting
+ Response updateConfigHelper(Callable<SetResult> updateMethod)
+ {
+ int attemps = 0;
+ SetResult setResult = null;
+ try {
+ while (attemps < UPDATE_NUM_RETRY) {
+ setResult = updateMethod.call();
+ if (setResult.isOk() || !setResult.isRetryable()) {
+ break;
+ }
+ attemps++;
+ updateRetryDelay();
+ }
+ }
Review comment:
The RetryUtils has a minimum wait time of min(60000ms, 1000ms * pow(2, n - 1))
The wait time is too long for this purpose as this API is called by the user / UI.
The wait time currently used here follows the same wait time as the `/druid-ext/basic-security/authentication` MetadataStorageConnector.compareAndSwap
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on a change in pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #11144:
URL: https://github.com/apache/druid/pull/11144#discussion_r629636342
##########
File path: server/src/main/java/org/apache/druid/server/http/CoordinatorCompactionConfigsResource.java
##########
@@ -162,27 +165,68 @@ public Response deleteCompactionConfig(
@Context HttpServletRequest req
)
{
- final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
- final Map<String, DataSourceCompactionConfig> configs = current
- .getCompactionConfigs()
- .stream()
- .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+ Callable<SetResult> callable = () -> {
+ final CoordinatorCompactionConfig current = CoordinatorCompactionConfig.current(manager);
+ final Map<String, DataSourceCompactionConfig> configs = current
+ .getCompactionConfigs()
+ .stream()
+ .collect(Collectors.toMap(DataSourceCompactionConfig::getDataSource, Function.identity()));
+
+ final DataSourceCompactionConfig config = configs.remove(dataSource);
+ if (config == null) {
+ return SetResult.fail(new NoSuchElementException("datasource not found"), false);
+ }
+
+ return manager.set(
+ CoordinatorCompactionConfig.CONFIG_KEY,
+ // Do database insert without swap if the current config is empty as this means the config may be null in the database
+ CoordinatorCompactionConfig.empty().equals(current) ? null : current,
+ CoordinatorCompactionConfig.from(current, ImmutableList.copyOf(configs.values())),
+ new AuditInfo(author, comment, req.getRemoteAddr())
+ );
+ };
+ return updateConfigHelper(callable);
+ }
- final DataSourceCompactionConfig config = configs.remove(dataSource);
- if (config == null) {
- return Response.status(Response.Status.NOT_FOUND).build();
+ @VisibleForTesting
+ Response updateConfigHelper(Callable<SetResult> updateMethod)
+ {
+ int attemps = 0;
+ SetResult setResult = null;
+ try {
+ while (attemps < UPDATE_NUM_RETRY) {
+ setResult = updateMethod.call();
+ if (setResult.isOk() || !setResult.isRetryable()) {
+ break;
+ }
+ attemps++;
+ updateRetryDelay();
+ }
+ }
Review comment:
I forgot about RetryUtils. Fixed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm merged pull request #11144: Make sure updating coordinator config is protected against race condition
Posted by GitBox <gi...@apache.org>.
maytasm merged pull request #11144:
URL: https://github.com/apache/druid/pull/11144
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org