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/12/03 04:01:14 UTC
[GitHub] [druid] zachjsh opened a new pull request #12021: Fix bad lookup config fails task
zachjsh opened a new pull request #12021:
URL: https://github.com/apache/druid/pull/12021
### Description
This PR fixes an issue in which if a lookup is configured incorreclty; does not serialize properly when being pulled by peon node, it causes the task to fail. The failure occurs because the peon and other leaf nodes (broker, historical), have retry logic that continues to retry the lookup loading for 3 minutes by default. The http listener thread on the peon task is not started until lookup loading completes, by default, the overlord waits 1 minute by default, to communicate with the peon task to get the task status, after which is orders the task to shut down, causing the ingestion task to fail.
To fix the issue, we catch the exception serialization error, and do not retry. Also fixed an issue in which a bad lookup config interferes with any other good lookup configs from being loaded.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ x] 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.
- [ x] 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.
- [x ] 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.
- [x ] 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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
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] zachjsh commented on a change in pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #12021:
URL: https://github.com/apache/druid/pull/12021#discussion_r763204700
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
Review comment:
@kfaraz, this util function is also used in `LookupListeningResource`, which is why I added to new utils class. I could add another function that just converts a single `LookupState<Object>`, but thought it would be good to have a function that converts a map, as this seems to be done in several places, so the code can be reused, without re-implementing the logic. Let me know what you think.
--
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@druid.apache.org
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] zachjsh commented on a change in pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #12021:
URL: https://github.com/apache/druid/pull/12021#discussion_r763382237
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
+{
+
+ private static final EmittingLogger LOG = new EmittingLogger(LookupUtils.class);
+
+ private LookupUtils()
+ {
+
+ }
+
+ /**
+ * Takes a map of String to Object, representing lookup name to generic lookup config, and attempts to construct
+ * a map from String to {@link LookupExtractorFactoryContainer}. Any lookup configs that are not able to be converted
+ * to {@link LookupExtractorFactoryContainer}, will be logged as warning, and will not be included in the map
+ * returned.
+ *
+ * @param lookupNameToGenericConfig The lookup generic config map.
+ * @param objectMapper The object mapper to use to convert bytes to {@link LookupExtractorFactoryContainer}
+ * @return
+ */
+ public static Map<String, LookupExtractorFactoryContainer> convertObjectMapToLookupExtractorFactoryContainerMapAndSkipErrors(
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.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
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] kfaraz commented on a change in pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
kfaraz commented on a change in pull request #12021:
URL: https://github.com/apache/druid/pull/12021#discussion_r762378594
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
+{
+
+ private static final EmittingLogger LOG = new EmittingLogger(LookupUtils.class);
+
+ private LookupUtils()
+ {
+
+ }
+
+ /**
+ * Takes a map of String to Object, representing lookup name to generic lookup config, and attempts to construct
+ * a map from String to {@link LookupExtractorFactoryContainer}. Any lookup configs that are not able to be converted
+ * to {@link LookupExtractorFactoryContainer}, will be logged as warning, and will not be included in the map
+ * returned.
+ *
+ * @param lookupNameToGenericConfig The lookup generic config map.
+ * @param objectMapper The object mapper to use to convert bytes to {@link LookupExtractorFactoryContainer}
+ * @return
+ */
+ public static Map<String, LookupExtractorFactoryContainer> convertObjectMapToLookupExtractorFactoryContainerMapAndSkipErrors(
Review comment:
Nit: Could this method be renamed to something like `tryConvertObjectMapToLookupConfigMap` to make it less verbose?
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
Review comment:
*Nit: style choice*
Instead of doing this in a new utils class, would it make sense to add this logic in `LookupReferencesManager` itself?
We could just pass the `LookupState<Object>` to the manager and let it do the rest.
--
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@druid.apache.org
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] zachjsh merged pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
zachjsh merged pull request #12021:
URL: https://github.com/apache/druid/pull/12021
--
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@druid.apache.org
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] zachjsh commented on a change in pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #12021:
URL: https://github.com/apache/druid/pull/12021#discussion_r763205243
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
+{
+
+ private static final EmittingLogger LOG = new EmittingLogger(LookupUtils.class);
+
+ private LookupUtils()
+ {
+
+ }
+
+ /**
+ * Takes a map of String to Object, representing lookup name to generic lookup config, and attempts to construct
+ * a map from String to {@link LookupExtractorFactoryContainer}. Any lookup configs that are not able to be converted
+ * to {@link LookupExtractorFactoryContainer}, will be logged as warning, and will not be included in the map
+ * returned.
+ *
+ * @param lookupNameToGenericConfig The lookup generic config map.
+ * @param objectMapper The object mapper to use to convert bytes to {@link LookupExtractorFactoryContainer}
+ * @return
+ */
+ public static Map<String, LookupExtractorFactoryContainer> convertObjectMapToLookupExtractorFactoryContainerMapAndSkipErrors(
Review comment:
Yeah I like your name better! Will change, just wanted to confirm other needed changes before making another commit.
--
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@druid.apache.org
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] zachjsh commented on a change in pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #12021:
URL: https://github.com/apache/druid/pull/12021#discussion_r763205243
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
+{
+
+ private static final EmittingLogger LOG = new EmittingLogger(LookupUtils.class);
+
+ private LookupUtils()
+ {
+
+ }
+
+ /**
+ * Takes a map of String to Object, representing lookup name to generic lookup config, and attempts to construct
+ * a map from String to {@link LookupExtractorFactoryContainer}. Any lookup configs that are not able to be converted
+ * to {@link LookupExtractorFactoryContainer}, will be logged as warning, and will not be included in the map
+ * returned.
+ *
+ * @param lookupNameToGenericConfig The lookup generic config map.
+ * @param objectMapper The object mapper to use to convert bytes to {@link LookupExtractorFactoryContainer}
+ * @return
+ */
+ public static Map<String, LookupExtractorFactoryContainer> convertObjectMapToLookupExtractorFactoryContainerMapAndSkipErrors(
Review comment:
Yeah this is a better name! Will change, just wanted to confirm other needed changes before making another commit.
--
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@druid.apache.org
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] zachjsh commented on a change in pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #12021:
URL: https://github.com/apache/druid/pull/12021#discussion_r763205243
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
+{
+
+ private static final EmittingLogger LOG = new EmittingLogger(LookupUtils.class);
+
+ private LookupUtils()
+ {
+
+ }
+
+ /**
+ * Takes a map of String to Object, representing lookup name to generic lookup config, and attempts to construct
+ * a map from String to {@link LookupExtractorFactoryContainer}. Any lookup configs that are not able to be converted
+ * to {@link LookupExtractorFactoryContainer}, will be logged as warning, and will not be included in the map
+ * returned.
+ *
+ * @param lookupNameToGenericConfig The lookup generic config map.
+ * @param objectMapper The object mapper to use to convert bytes to {@link LookupExtractorFactoryContainer}
+ * @return
+ */
+ public static Map<String, LookupExtractorFactoryContainer> convertObjectMapToLookupExtractorFactoryContainerMapAndSkipErrors(
Review comment:
Yeah your suggestion is a better name! Will change, just wanted to confirm other needed changes before making another commit.
--
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@druid.apache.org
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] kfaraz commented on a change in pull request #12021: Fix bad lookup config fails task
Posted by GitBox <gi...@apache.org>.
kfaraz commented on a change in pull request #12021:
URL: https://github.com/apache/druid/pull/12021#discussion_r763345186
##########
File path: server/src/main/java/org/apache/druid/query/lookup/LookupUtils.java
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.lookup;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.Maps;
+import org.apache.druid.java.util.emitter.EmittingLogger;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * Utility class for lookup related things
+ */
+public class LookupUtils
Review comment:
Yes, makes sense, @zachjsh
--
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@druid.apache.org
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