You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/05/22 08:22:08 UTC
[GitHub] [incubator-doris] SaintBacchus opened a new pull request, #9725: [Enhancement] Add a ProfileStorage interface to support record all the querylog in persist storage.
SaintBacchus opened a new pull request, #9725:
URL: https://github.com/apache/incubator-doris/pull/9725
# Proposed changes
Related with [9724](https://github.com/apache/incubator-doris/issues/9724)
## Problem Summary:
Abstract an interface named ProfileStorage with write and read method and impl the InMemoryProfileStorage which will keep the same behaviour with now.
## Checklist(Required)
1. Does it affect the original behavior: (No)
2. Has unit tests been added: (No Need)
3. Has document been added or modified: (No Need)
4. Does it need to update dependencies: (No)
5. Are there any changes that cannot be rolled back: (No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] SaintBacchus closed pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
SaintBacchus closed pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
URL: https://github.com/apache/incubator-doris/pull/9725
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] SaintBacchus commented on a diff in pull request #9725: [Enhancement] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
SaintBacchus commented on code in PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#discussion_r878979650
##########
fe/fe-core/src/main/java/org/apache/doris/common/Pair.java:
##########
@@ -22,14 +22,15 @@
import com.google.gson.annotations.SerializedName;
+import java.io.Serializable;
import java.util.Comparator;
/**
* The equivalent of C++'s std::pair<>.
*
* Notice: When using Pair for persistence, users need to guarantee that F and S can be serialized through Gson
*/
-public class Pair<F, S> {
+public class Pair<F, S> implements Serializable {
Review Comment:
I planned to seriable profile to bytes. And now I will remove this
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] stalary commented on pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
stalary commented on PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#issuecomment-1135515892
> @stalary In the lastest commit I separate the interface `getProfile(queryId)` into two parts of `getProfileContent` and `getProfileBuilder`. Since if we want to store the record in a db system and read the big field to construct the `ProfileBuilder` instance, it maybe a heave thing if we only want to list the record or get the query profile content.
>
> But it maybe take a new disadvantage that if we want to add more filed into query profile, it has to add a new method instead of add a filed in `ProfileElemt`. It may cause the number of methods loss control.
>
> Which one do you think is better?
I think it's ok to return enough data at the bottom level for the top level to choose. This will be easier to extend later when we support more storage methods. So in my opinion, split is not necessary.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] stalary commented on a diff in pull request #9725: [Enhancement] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
stalary commented on code in PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#discussion_r878959299
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/ProfileManager.java:
##########
@@ -104,160 +90,60 @@ public static ProfileManager getInstance() {
}
private ProfileManager() {
- lock = new ReentrantReadWriteLock(true);
- readLock = lock.readLock();
- writeLock = lock.writeLock();
- queryIdDeque = new LinkedList<>();
- queryIdToProfileMap = new ConcurrentHashMap<>();
+ storage = new InMemoryProfileStorage();
Review Comment:
Will we use variables to decide which storage to use in the future?
##########
fe/fe-core/src/main/java/org/apache/doris/common/Pair.java:
##########
@@ -22,14 +22,15 @@
import com.google.gson.annotations.SerializedName;
+import java.io.Serializable;
import java.util.Comparator;
/**
* The equivalent of C++'s std::pair<>.
*
* Notice: When using Pair for persistence, users need to guarantee that F and S can be serialized through Gson
*/
-public class Pair<F, S> {
+public class Pair<F, S> implements Serializable {
Review Comment:
Why change it.
##########
fe/fe-core/src/main/java/org/apache/doris/common/profile/InMemoryProfileStorage.java:
##########
@@ -0,0 +1,159 @@
+// 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.doris.common.profile;
+
+import org.apache.doris.common.util.ProfileManager;
+import org.apache.doris.common.util.ProfileManager.ProfileElement;
+import org.apache.doris.common.util.ProfileManager.ProfileType;
+import org.apache.doris.common.util.RuntimeProfile;
+
+import com.google.common.base.Strings;
+import com.google.common.collect.Lists;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Deque;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;
+
+/**
+ * An inMemory impl of profile storage. It will only keep 100 records in memory.
+ * If you want to keep more query records, you can use other impl of profile storage.
+ */
+public class InMemoryProfileStorage implements ProfileStorage {
+ private static final Logger LOG = LogManager.getLogger(InMemoryProfileStorage.class);
+
+ private static final int ARRAY_SIZE = 100;
Review Comment:
Maybe we can control the size by variables.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] stalary commented on pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
stalary commented on PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#issuecomment-1134551330
Can you supplement the unit tests.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] SaintBacchus commented on pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
SaintBacchus commented on PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#issuecomment-1135530818
OK, I revert the last commit and do the performence improve in the later worker.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] stalary commented on pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
stalary commented on PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#issuecomment-1134778896
LGTM
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] SaintBacchus commented on a diff in pull request #9725: [Enhancement] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
SaintBacchus commented on code in PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#discussion_r878984613
##########
fe/fe-core/src/main/java/org/apache/doris/common/profile/InMemoryProfileStorage.java:
##########
@@ -0,0 +1,159 @@
+// 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.doris.common.profile;
+
+import org.apache.doris.common.util.ProfileManager;
+import org.apache.doris.common.util.ProfileManager.ProfileElement;
+import org.apache.doris.common.util.ProfileManager.ProfileType;
+import org.apache.doris.common.util.RuntimeProfile;
+
+import com.google.common.base.Strings;
+import com.google.common.collect.Lists;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Deque;
+import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;
+
+/**
+ * An inMemory impl of profile storage. It will only keep 100 records in memory.
+ * If you want to keep more query records, you can use other impl of profile storage.
+ */
+public class InMemoryProfileStorage implements ProfileStorage {
+ private static final Logger LOG = LogManager.getLogger(InMemoryProfileStorage.class);
+
+ private static final int ARRAY_SIZE = 100;
Review Comment:
This is a good idea. But this size is also hard code in web ui which plan to change in the last pr
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] SaintBacchus commented on a diff in pull request #9725: [Enhancement] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
SaintBacchus commented on code in PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#discussion_r878981108
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/ProfileManager.java:
##########
@@ -104,160 +90,60 @@ public static ProfileManager getInstance() {
}
private ProfileManager() {
- lock = new ReentrantReadWriteLock(true);
- readLock = lock.readLock();
- writeLock = lock.writeLock();
- queryIdDeque = new LinkedList<>();
- queryIdToProfileMap = new ConcurrentHashMap<>();
+ storage = new InMemoryProfileStorage();
Review Comment:
@stalary yes, I plan to use variables in next pr, because there is only one impl of it
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] SaintBacchus commented on pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
SaintBacchus commented on PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#issuecomment-1135500847
@stalary In the lastest commit I separate the interface `getProfile(queryId)` into two parts of `getProfileContent` and `getProfileBuilder`.
Since if we want to store the record in a db system and read the big field to construct the `ProfileBuilder` instance, it maybe a heave thing if we only want to list the record or get the query profile content.
But it maybe take a new disadvantage that if we want to add more filed into query profile, it has to add a new method instead of add a filed in `ProfileElemt`. It may cause the number of methods loss control.
Which one do you think is better?
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] stalary commented on pull request #9725: [Enhancement][Step1] Add a ProfileStorage interface to support record all the querylog in persist storage.
Posted by GitBox <gi...@apache.org>.
stalary commented on PR #9725:
URL: https://github.com/apache/incubator-doris/pull/9725#issuecomment-1135515003
I think it's ok to return enough data at the bottom level for the top level to choose. This will be easier to extend later when we support more storage methods.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org