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 2021/05/27 05:43:10 UTC
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6977: Enhance debug endpoint for table debugging.
Jackie-Jiang commented on a change in pull request #6977:
URL: https://github.com/apache/incubator-pinot/pull/6977#discussion_r640297369
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/debug/TableDebugInfo.java
##########
@@ -102,21 +113,71 @@ public int getNumBrokers() {
return _brokerDebugInfos;
}
+ @JsonPropertyOrder({"segmentName", "serverState"})
public static class SegmentDebugInfo {
private final String _segmentName;
- private final List<IsEvState> _states;
- public SegmentDebugInfo(String name, List<IsEvState> states) {
- _segmentName = name;
- _states = states;
+ private final Map<String, SegmentServerState> _serverStateMap;
+
+ @JsonCreator
+ public SegmentDebugInfo(@JsonProperty("segmentName") String segmentName,
+ @JsonProperty("serverState") Map<String, SegmentServerState> segmentServerState) {
+ _segmentName = segmentName;
+ _serverStateMap = segmentServerState;
}
public String getSegmentName() {
return _segmentName;
}
- public List<IsEvState> getSegmentStateInServer() {
- return _states;
+ public Map<String, SegmentServerState> getServerState() {
+ return _serverStateMap;
+ }
+ }
+
+ /**
+ * This class represents the state of segment on the server:
+ *
+ * <ul>
+ * <li>Ideal State vs External view.</li>
+ * <li>Segment related errors and consumer information.</li>
+ * <li>Segment size.</li>
+ * </ul>
+ */
+ public static class SegmentServerState {
Review comment:
Probably just `SegmentState` which is more concise?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentErrorInfo.java
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.pinot.common.restlet.resources;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.Locale;
+import java.util.TimeZone;
+import org.apache.commons.lang3.exception.ExceptionUtils;
+
+
+/**
+ * This class is used to represent errors related to a segment.
+ */
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class SegmentErrorInfo {
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss z";
+
+ private final String _timeStamp;
+ private final String _error;
+ private final String _exception;
+
+ /**
+ *
+ * @param exception Exception object
+ * @param errorMessage Error Message
+ * @param timeStampInMillisSinceEpoch Timestamp of the error
+ */
+ public SegmentErrorInfo(Exception exception, String errorMessage, long timeStampInMillisSinceEpoch) {
Review comment:
```suggestion
public SegmentErrorInfo(Exception exception, String errorMessage, long timestampMs) {
```
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentErrorInfo.java
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.pinot.common.restlet.resources;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.Locale;
+import java.util.TimeZone;
+import org.apache.commons.lang3.exception.ExceptionUtils;
+
+
+/**
+ * This class is used to represent errors related to a segment.
+ */
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class SegmentErrorInfo {
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss z";
+
+ private final String _timeStamp;
Review comment:
We usually treat "timestamp" as a single word. Same for other places
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java
##########
@@ -228,4 +240,20 @@ public String getTableName() {
public File getTableDataDir() {
return _indexDir;
}
+
+ @Override
+ public void addSegmentError(String segmentName, SegmentErrorInfo segmentErrorInfo) {
+ _errorCache.put(new Pair<>(_tableNameWithType, segmentName), segmentErrorInfo);
+ }
+
+ @Override
+ public Map<String, SegmentErrorInfo> getSegmentErrors() {
+ if (_errorCache == null) {
+ return Collections.emptyMap();
+ } else {
+ // Filter out entries that match the table name.
+ return _errorCache.asMap().entrySet().stream().filter(map -> map.getKey().getFirst().equals(_tableNameWithType))
Review comment:
Avoid `stream` apis for performance concern
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentErrorInfo.java
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.pinot.common.restlet.resources;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.Locale;
+import java.util.TimeZone;
+import org.apache.commons.lang3.exception.ExceptionUtils;
+
+
+/**
+ * This class is used to represent errors related to a segment.
+ */
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class SegmentErrorInfo {
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss z";
+
+ private final String _timeStamp;
+ private final String _error;
+ private final String _exception;
+
+ /**
+ *
+ * @param exception Exception object
+ * @param errorMessage Error Message
+ * @param timeStampInMillisSinceEpoch Timestamp of the error
+ */
+ public SegmentErrorInfo(Exception exception, String errorMessage, long timeStampInMillisSinceEpoch) {
+ _timeStamp = epochToSDF(timeStampInMillisSinceEpoch);
+ _error = errorMessage;
+ _exception = (exception != null) ? ExceptionUtils.getStackTrace(exception) : null;
+ }
+
+ /**
+ * This constructor is specifically for JSON ser/de.
+ *
+ * @param exception Exception stack trace
+ * @param error Error message
+ * @param timeStamp Time stamp of the error in Simple Date Format.
+ *
+ */
+ @JsonCreator
+ public SegmentErrorInfo(@JsonProperty("exception") String exception, @JsonProperty("error") String error,
+ @JsonProperty("timeStamp") String timeStamp) {
+ _timeStamp = timeStamp;
+ _error = error;
+ _exception = exception;
+ }
+
+ public String getTimeStamp() {
+ return _timeStamp;
+ }
+
+ public String getError() {
+ return _error;
+ }
+
+ public String getException() {
+ return _exception;
+ }
+
+ /**
+ * Utility function to convert epoch in millis to SDF of form "yyyy-MM-dd HH:mm:ss z".
+ *
+ * @param millisSinceEpoch Time in millis to convert
+ * @return SDF equivalent
+ */
+ private static String epochToSDF(long millisSinceEpoch) {
+ SimpleDateFormat sdf = new SimpleDateFormat(DATE_FORMAT, Locale.getDefault());
Review comment:
We should put the SDF as a constant without initializing the format for every call
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentErrorInfo.java
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.pinot.common.restlet.resources;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.Locale;
+import java.util.TimeZone;
+import org.apache.commons.lang3.exception.ExceptionUtils;
+
+
+/**
+ * This class is used to represent errors related to a segment.
+ */
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class SegmentErrorInfo {
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss z";
+
+ private final String _timeStamp;
+ private final String _error;
+ private final String _exception;
Review comment:
`_stackTrace` for clarity?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentErrorInfo.java
##########
@@ -0,0 +1,94 @@
+/**
+ * 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.pinot.common.restlet.resources;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.Locale;
+import java.util.TimeZone;
+import org.apache.commons.lang3.exception.ExceptionUtils;
+
+
+/**
+ * This class is used to represent errors related to a segment.
+ */
+@SuppressWarnings("unused")
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class SegmentErrorInfo {
+ private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss z";
+
+ private final String _timeStamp;
+ private final String _error;
Review comment:
`_errorMessage` for clarity?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentServerDebugInfo.java
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pinot.common.restlet.resources;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+
+
+/**
+ * This class represents the server-side debug information for a segment.
+ */
+@JsonIgnoreProperties(ignoreUnknown = true)
+public class SegmentServerDebugInfo {
Review comment:
I assume this name it picked to differentiate from the controller `SegmentDebugInfo`? Is `ServerSegmentDebugInfo` better?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableDebugResource.java
##########
@@ -156,38 +177,94 @@ private TableDebugInfo debugTable(PinotHelixResourceManager pinotHelixResourceMa
*
* @param pinotHelixResourceManager Helix Resource Manager
* @param tableNameWithType Name of table with type
+ * @param verbose If true, returns all segment info, else, only segments with errors
* @return Debug information for segments
*/
private List<TableDebugInfo.SegmentDebugInfo> debugSegments(PinotHelixResourceManager pinotHelixResourceManager,
- String tableNameWithType) {
+ String tableNameWithType, boolean verbose) {
ExternalView externalView = pinotHelixResourceManager.getTableExternalView(tableNameWithType);
IdealState idealState = pinotHelixResourceManager.getTableIdealState(tableNameWithType);
- List<TableDebugInfo.SegmentDebugInfo> segmentDebugInfos = new ArrayList<>();
+ List<TableDebugInfo.SegmentDebugInfo> result = new ArrayList<>();
if (idealState == null) {
- return segmentDebugInfos;
+ return result;
+ }
+
+ int serverRequestTimeoutMs = _controllerConf.getServerAdminRequestTimeoutSeconds() * 1000;
+ final Map<String, List<String>> serverToSegments =
+ _pinotHelixResourceManager.getServerToSegmentsMap(tableNameWithType);
+ Map<String, Map<String, SegmentServerDebugInfo>> segmentsDebugInfoFromServers;
+
+ BiMap<String, String> serverToEndpoints;
+ try {
+ serverToEndpoints = _pinotHelixResourceManager.getDataInstanceAdminEndpoints(serverToSegments.keySet());
+ } catch (InvalidConfigException e) {
+ throw new WebApplicationException(
+ "Caught exception when getting segment debug info for table: " + tableNameWithType);
}
+ segmentsDebugInfoFromServers =
+ getSegmentsDebugInfoFromServers(tableNameWithType, serverToEndpoints, serverRequestTimeoutMs);
+
for (Map.Entry<String, Map<String, String>> segmentMapEntry : idealState.getRecord().getMapFields().entrySet()) {
- String segment = segmentMapEntry.getKey();
- Map<String, String> instanceStateMap = segmentMapEntry.getValue();
+ String segmentName = segmentMapEntry.getKey();
+ Map<String, String> segmentIsMap = segmentMapEntry.getValue();
+
+ Map<String, TableDebugInfo.SegmentServerState> segmentServerState = new HashMap<>();
+ for (Map.Entry<String, Map<String, SegmentServerDebugInfo>> segmentEntry : segmentsDebugInfoFromServers
+ .entrySet()) {
+ String instanceName = segmentEntry.getKey();
- List<TableDebugInfo.IsEvState> isEvStates = new ArrayList<>();
- for (Map.Entry<String, String> entry : instanceStateMap.entrySet()) {
- String serverName = entry.getKey();
- String isState = entry.getValue();
+ String isState = segmentIsMap.get(instanceName);
+ String evState = (externalView != null) ? externalView.getStateMap(segmentName).get(instanceName) : "null";
+ Map<String, SegmentServerDebugInfo> segmentServerDebugInfoMap = segmentEntry.getValue();
- String evState = (externalView != null) ? externalView.getStateMap(segment).get(serverName) : "null";
- if (!isState.equals(evState)) {
- isEvStates.add(new TableDebugInfo.IsEvState(serverName, isState, evState));
+ if (verbose || segmentHasErrors(segmentServerDebugInfoMap, isState, evState)) {
+ SegmentServerDebugInfo segmentServerDebugInfo = segmentServerDebugInfoMap.get(segmentName);
+
+ segmentServerState.put(instanceName,
+ new TableDebugInfo.SegmentServerState(isState, evState, segmentServerDebugInfo.getSegmentSize(),
+ segmentServerDebugInfo.getConsumerInfo(), segmentServerDebugInfo.getErrorInfo()));
}
}
- if (isEvStates.size() > 0) {
- segmentDebugInfos.add(new TableDebugInfo.SegmentDebugInfo(segment, isEvStates));
+ if (!segmentServerState.isEmpty()) {
+ result.add(new TableDebugInfo.SegmentDebugInfo(segmentName, segmentServerState));
}
}
- return segmentDebugInfos;
+
+ return result;
+ }
+
+ /**
+ * Helper method to check if a segment has any errors/issues.
+ *
+ * @param segmentServerDebugInfoMap Map from instanceName to segmentDebugInfo
+ * @param isState ideal state of segment
+ * @param evState external view of segment
+ * @return True if there's any error/issue for the segment, false otherwise.
+ */
+ private boolean segmentHasErrors(Map<String, SegmentServerDebugInfo> segmentServerDebugInfoMap, String isState,
+ String evState) {
+ if (!isState.equals(evState)) {
+ return true;
Review comment:
Should we count this as error? It is normal case during state transition
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/api/resources/ServerResourceUtils.java
##########
@@ -0,0 +1,58 @@
+/**
+ * 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.pinot.server.api.resources;
+
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.core.data.manager.InstanceDataManager;
+import org.apache.pinot.segment.local.data.manager.TableDataManager;
+import org.apache.pinot.server.starter.ServerInstance;
+
+
+/**
+ * Utility class for Server resources.
+ */
+public class ServerResourceUtils {
+
+ // Disable instantiation.
+ private ServerResourceUtils() {
+
+ }
+
+ public static TableDataManager checkGetTableDataManager(ServerInstance serverInstance, String tableName) {
+ InstanceDataManager dataManager = checkGetInstanceDataManager(serverInstance);
+ TableDataManager tableDataManager = dataManager.getTableDataManager(tableName);
+ if (tableDataManager == null) {
+ throw new WebApplicationException("Table " + tableName + " does not exist", Response.Status.NOT_FOUND);
+ }
+ return tableDataManager;
+ }
+
+ public static InstanceDataManager checkGetInstanceDataManager(ServerInstance serverInstance) {
Review comment:
(Maybe out of the scope of this PR, but since this PR refactors this) Seems we don't need these extra checks. When the rest API is up, both `serverInstance` and `instanceDataManager` will never 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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org