You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "kezhenxu94 (via GitHub)" <gi...@apache.org> on 2023/03/16 07:12:03 UTC
[GitHub] [skywalking] kezhenxu94 opened a new pull request, #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
kezhenxu94 opened a new pull request, #10544:
URL: https://github.com/apache/skywalking/pull/10544
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
<!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10366.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138324380
##########
oap-server/server-storage-plugin/storage-shardingsphere-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/shardingsphere/ShardingRulesOperator.java:
##########
@@ -157,23 +152,35 @@ private ShardingRule.ShardingRuleBuilder buildShardingRule(ShardingRule.Sharding
buildDataNodes(builder, tableName, dataSources, ttl, currentDate);
buildDatabaseStrategy(builder, dsShardingColumn, dataSources.size());
- switch (shardingAlgorithm) {
- case TIME_SEC_RANGE_SHARDING_ALGORITHM:
- buildTimeRangeTableStrategy(builder, tableShardingColumn, TIME_SEC_RANGE_SHARDING_EXPRESSION);
- break;
- case TIME_MIN_RANGE_SHARDING_ALGORITHM:
- buildTimeRangeTableStrategy(builder, tableShardingColumn, TIME_MIN_RANGE_SHARDING_EXPRESSION);
- break;
- case TIME_RELATIVE_ID_SHARDING_ALGORITHM:
- buildExpressionTableStrategy(builder, tableName, tableShardingColumn,
- TIME_RELATIVE_ID_SHARDING_EXPRESSION);
- break;
- case TIME_BUCKET_SHARDING_ALGORITHM:
- buildExpressionTableStrategy(builder, tableName, tableShardingColumn, TIME_BUCKET_SHARDING_EXPRESSION);
- break;
- default:
- throw new UnexpectedException("Unsupported sharding algorithm " + shardingAlgorithm);
- }
Review Comment:
> We don't need these anymore? But I don't see you remove them from the top of the Class code.
Now removed.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139906056
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
How about we have an `expireAfterRead` and do manually remove when `table not found` SQL exception? If possible, this could reduce more payload.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139805415
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
> I think failing to read from Database in sync may be not worth it.
>
> 1. It could be slow
> 2. If this kind of query keeps happening, we cost a lot of resources in the database.
>
> Instead, moving the table cache from `DeleteDao` to JDBC global scope should help. After all, it only read once period.
Caching these brings other problems, when the tables are deleted in another OAP node, we don't get notified, and still consider this table exists, then we might send queries to an inexistent table, which causes errors.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1471462150
> Should we remove the tidb provider with relative docs(maybe keep a little in MySQL section)? As the rolling and drop table as TTL, the TiDB provider doesn't seem necessary.
Do you want to keep the tests for TiDB?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1471532158
> [Disable SS related tests](https://github.com/apache/skywalking/pull/10544/commits/e59626b6a51b8f596bff0379aa270e0b797289e6)
Does this mean we will skip SS implementation for this 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1471468939
> > Should we remove the tidb provider with relative docs(maybe keep a little in MySQL section)? As the rolling and drop table as TTL, the TiDB provider doesn't seem necessary.
>
> Do you want to keep the tests for TiDB?
I think we are safe to remove. We don't have obligations to test compatibility.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139930376
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
> How about we have an `expireAfterRead` and do manually remove when `table not found` SQL exception? If possible, this could reduce more payload.
Hi, `expireAfterWrite` --> `expireAfterRead` sounds reasonable to me. However manually invalidating the cache doesn't make any sense to me, manually invalidating the cache only cause the `LoadingCache` to invoke the `load` method and tries to fill the cache again and thus increase the load, it **doesn't** sounds like "hey, if there is no entry in cache, don't do anything", this is how a loading cache works: load in the cache first, if absent, load from a remote source or load through a heavy computational operations, by manually invalidating the cache you are requesting the heavy computational operations over and over again because the cache entry is invalidated manually.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1471549122
With the new mechanism, actually, we could work better on the traditional database, and many transparent options to be distributed MySQL/PostgreSQL DBs. I am considering the reasons for keeping or removing 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139821488
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
I think we should control the query start time and end time within TTL rather than to query table if exist.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1472997312
@kezhenxu94 As you are not implementing the ShardingSphere in this PR, please remove all relative codes and tests. If one day, we will add it back, we could pick from 9.4.0 branch.
I don't hope we have confused change logs about 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138596144
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
OK, I noticed this is another misguide by naming. This method is from TTL, so, most likely we should rename to `getExistingTables` or similar.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1473724376
@wankai123 Please take a look.
FYI @mrproliu I expect this would be merged soon, and your block will be gone.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1471439803
Should we remove the tidb provider with relative docs(maybe keep a little in MySQL section)? As the rolling and drop table as TTL, the TiDB provider doesn't seem 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139930376
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
> How about we have an `expireAfterRead` and do manually remove when `table not found` SQL exception? If possible, this could reduce more payload.
Hi, `expireAfterWrite` --> `expireAfterRead` sounds reasonable to me. However manually invalidating the cache doesn't make any sense to me, manually invalidating the cache only cause the `LoadingCache` to invoke the `load` method and tries to fill the cache again and thus increase the load, it **doesn't** sounds like "hey, if there is no entry in cache, don't do anything", this is how a loading cache works: load in the cache first, if absent, load from a remote source or load through a heavy computational operation, by manually invalidating the cache you are requesting the heavy computational operations over and over again because the cache entry is invalidated manually.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139936541
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
Make sense for `manually remove`.
Meanwhile, considering at the beginning, only the current date index is created, then N - 7 will be always absent, right? Should we maybe put an Optional<TableStatus> in the cache in case of always doing metadata queries?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139821821
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
It is worth to a cache without loader mechanism to deal with this.
The metadata reading is low performance. We should not include it into query process.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139968833
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
Done
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139812759
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/dao/JDBCHistoryDeleteDAO.java:
##########
@@ -18,66 +18,100 @@
package org.apache.skywalking.oap.server.storage.plugin.jdbc.common.dao;
-import java.io.IOException;
-import java.sql.Connection;
-import java.sql.SQLException;
+import lombok.RequiredArgsConstructor;
+import lombok.SneakyThrows;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
import org.apache.skywalking.oap.server.core.storage.IHistoryDeleteDAO;
import org.apache.skywalking.oap.server.core.storage.model.Model;
-import org.apache.skywalking.oap.server.core.storage.model.SQLDatabaseModelExtension;
-import org.apache.skywalking.oap.server.library.client.jdbc.JDBCClientException;
-import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCHikariCPClient;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
import org.apache.skywalking.oap.server.storage.plugin.jdbc.SQLBuilder;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.JDBCTableInstaller;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.TableHelper;
import org.joda.time.DateTime;
-import lombok.RequiredArgsConstructor;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
+@Slf4j
@RequiredArgsConstructor
public class JDBCHistoryDeleteDAO implements IHistoryDeleteDAO {
- private final JDBCHikariCPClient jdbcClient;
+ private final JDBCClient jdbcClient;
+ private final TableHelper tableHelper;
+ private final JDBCTableInstaller modelInstaller;
+
+ private final Map<Pair<Model, String>, Boolean> tableExistenceCache = new ConcurrentHashMap<>();
@Override
- public void deleteHistory(Model model, String timeBucketColumnName, int ttl) throws IOException {
- SQLBuilder dataDeleteSQL = new SQLBuilder("delete from " + model.getName() + " where ")
- .append(timeBucketColumnName).append("<= ? ")
- .append(" and ")
- .append(timeBucketColumnName).append(">= ? ");
+ @SneakyThrows
+ public void deleteHistory(Model model, String timeBucketColumnName, int ttl) {
+ final var endTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ final var startTimeBucket = endTimeBucket - ttl;
+ log.info(
+ "Deleting history data, ttl: {}, now: {}. Keep [{}, {}]",
+ ttl,
+ System.currentTimeMillis(),
+ startTimeBucket,
+ endTimeBucket
+ );
+
+ final var ttlTables = tableHelper.getTablesForRead(model.getName(), startTimeBucket, endTimeBucket);
+ final var tablesToDrop = new HashSet<String>();
- try (Connection connection = jdbcClient.getConnection()) {
- long deadline;
- long minTime;
- if (model.isRecord()) {
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmmss"));
- minTime = 1000_00_00_00_00_00L;
- } else {
- switch (model.getDownsampling()) {
- case Minute:
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmm"));
- minTime = 1000_00_00_00_00L;
- break;
- case Hour:
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHH"));
- minTime = 1000_00_00_00L;
- break;
- case Day:
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMdd"));
- minTime = 1000_00_00L;
- break;
- default:
- return;
- }
+ try (final var conn = jdbcClient.getConnection();
+ final var result = conn.getMetaData().getTables(null, null, TableHelper.getTableName(model) + "%", new String[]{"TABLE"})) {
+ while (result.next()) {
+ tablesToDrop.add(result.getString("TABLE_NAME"));
}
- jdbcClient.executeUpdate(connection, dataDeleteSQL.toString(), deadline, minTime);
- // Delete additional tables
- for (SQLDatabaseModelExtension.AdditionalTable additionalTable : model.getSqlDBModelExtension()
- .getAdditionalTables()
- .values()) {
- SQLBuilder additionalTableDeleteSQL = new SQLBuilder("delete from " + additionalTable.getName() + " where ")
- .append(timeBucketColumnName).append("<= ? ")
- .append(" and ")
- .append(timeBucketColumnName).append(">= ? ");
- jdbcClient.executeUpdate(connection, additionalTableDeleteSQL.toString(), deadline, minTime);
+ }
+
+ ttlTables.forEach(tablesToDrop::remove);
+ tablesToDrop.removeIf(it -> !it.matches(".*_\\d{8}"));
+ for (String table : tablesToDrop) {
+ final var dropSql = new SQLBuilder("drop table if exists ").append(table);
+ jdbcClient.executeUpdate(dropSql.toString());
+ }
+
+ long deadline;
+ long minTime;
+ if (model.isRecord()) {
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmmss"));
+ minTime = 1000_00_00_00_00_00L;
+ } else {
+ switch (model.getDownsampling()) {
+ case Minute:
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmm"));
+ minTime = 1000_00_00_00_00L;
+ break;
+ case Hour:
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHH"));
+ minTime = 1000_00_00_00L;
+ break;
+ case Day:
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMdd"));
+ minTime = 1000_00_00L;
+ break;
+ default:
+ return;
}
- } catch (JDBCClientException | SQLException e) {
- throw new IOException(e.getMessage(), e);
+ }
+ // Delete data in additional tables
+ for (final var additionalTable : model.getSqlDBModelExtension().getAdditionalTables().values()) {
+ SQLBuilder additionalTableDeleteSQL = new SQLBuilder("delete from " + additionalTable.getName() + " where ")
Review Comment:
Done
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/dao/JDBCHistoryDeleteDAO.java:
##########
@@ -18,66 +18,100 @@
package org.apache.skywalking.oap.server.storage.plugin.jdbc.common.dao;
-import java.io.IOException;
-import java.sql.Connection;
-import java.sql.SQLException;
+import lombok.RequiredArgsConstructor;
+import lombok.SneakyThrows;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
import org.apache.skywalking.oap.server.core.storage.IHistoryDeleteDAO;
import org.apache.skywalking.oap.server.core.storage.model.Model;
-import org.apache.skywalking.oap.server.core.storage.model.SQLDatabaseModelExtension;
-import org.apache.skywalking.oap.server.library.client.jdbc.JDBCClientException;
-import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCHikariCPClient;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
import org.apache.skywalking.oap.server.storage.plugin.jdbc.SQLBuilder;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.JDBCTableInstaller;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.TableHelper;
import org.joda.time.DateTime;
-import lombok.RequiredArgsConstructor;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
+@Slf4j
@RequiredArgsConstructor
public class JDBCHistoryDeleteDAO implements IHistoryDeleteDAO {
- private final JDBCHikariCPClient jdbcClient;
+ private final JDBCClient jdbcClient;
+ private final TableHelper tableHelper;
+ private final JDBCTableInstaller modelInstaller;
+
+ private final Map<Pair<Model, String>, Boolean> tableExistenceCache = new ConcurrentHashMap<>();
@Override
- public void deleteHistory(Model model, String timeBucketColumnName, int ttl) throws IOException {
- SQLBuilder dataDeleteSQL = new SQLBuilder("delete from " + model.getName() + " where ")
- .append(timeBucketColumnName).append("<= ? ")
- .append(" and ")
- .append(timeBucketColumnName).append(">= ? ");
+ @SneakyThrows
+ public void deleteHistory(Model model, String timeBucketColumnName, int ttl) {
+ final var endTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
Review Comment:
Done
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
Review Comment:
Done
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138578783
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/dao/JDBCHistoryDeleteDAO.java:
##########
@@ -18,66 +18,100 @@
package org.apache.skywalking.oap.server.storage.plugin.jdbc.common.dao;
-import java.io.IOException;
-import java.sql.Connection;
-import java.sql.SQLException;
+import lombok.RequiredArgsConstructor;
+import lombok.SneakyThrows;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
import org.apache.skywalking.oap.server.core.storage.IHistoryDeleteDAO;
import org.apache.skywalking.oap.server.core.storage.model.Model;
-import org.apache.skywalking.oap.server.core.storage.model.SQLDatabaseModelExtension;
-import org.apache.skywalking.oap.server.library.client.jdbc.JDBCClientException;
-import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCHikariCPClient;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
import org.apache.skywalking.oap.server.storage.plugin.jdbc.SQLBuilder;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.JDBCTableInstaller;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.TableHelper;
import org.joda.time.DateTime;
-import lombok.RequiredArgsConstructor;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
+@Slf4j
@RequiredArgsConstructor
public class JDBCHistoryDeleteDAO implements IHistoryDeleteDAO {
- private final JDBCHikariCPClient jdbcClient;
+ private final JDBCClient jdbcClient;
+ private final TableHelper tableHelper;
+ private final JDBCTableInstaller modelInstaller;
+
+ private final Map<Pair<Model, String>, Boolean> tableExistenceCache = new ConcurrentHashMap<>();
@Override
- public void deleteHistory(Model model, String timeBucketColumnName, int ttl) throws IOException {
- SQLBuilder dataDeleteSQL = new SQLBuilder("delete from " + model.getName() + " where ")
- .append(timeBucketColumnName).append("<= ? ")
- .append(" and ")
- .append(timeBucketColumnName).append(">= ? ");
+ @SneakyThrows
+ public void deleteHistory(Model model, String timeBucketColumnName, int ttl) {
+ final var endTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ final var startTimeBucket = endTimeBucket - ttl;
+ log.info(
+ "Deleting history data, ttl: {}, now: {}. Keep [{}, {}]",
+ ttl,
+ System.currentTimeMillis(),
+ startTimeBucket,
+ endTimeBucket
+ );
+
+ final var ttlTables = tableHelper.getTablesForRead(model.getName(), startTimeBucket, endTimeBucket);
+ final var tablesToDrop = new HashSet<String>();
- try (Connection connection = jdbcClient.getConnection()) {
- long deadline;
- long minTime;
- if (model.isRecord()) {
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmmss"));
- minTime = 1000_00_00_00_00_00L;
- } else {
- switch (model.getDownsampling()) {
- case Minute:
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmm"));
- minTime = 1000_00_00_00_00L;
- break;
- case Hour:
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHH"));
- minTime = 1000_00_00_00L;
- break;
- case Day:
- deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMdd"));
- minTime = 1000_00_00L;
- break;
- default:
- return;
- }
+ try (final var conn = jdbcClient.getConnection();
+ final var result = conn.getMetaData().getTables(null, null, TableHelper.getTableName(model) + "%", new String[]{"TABLE"})) {
+ while (result.next()) {
+ tablesToDrop.add(result.getString("TABLE_NAME"));
}
- jdbcClient.executeUpdate(connection, dataDeleteSQL.toString(), deadline, minTime);
- // Delete additional tables
- for (SQLDatabaseModelExtension.AdditionalTable additionalTable : model.getSqlDBModelExtension()
- .getAdditionalTables()
- .values()) {
- SQLBuilder additionalTableDeleteSQL = new SQLBuilder("delete from " + additionalTable.getName() + " where ")
- .append(timeBucketColumnName).append("<= ? ")
- .append(" and ")
- .append(timeBucketColumnName).append(">= ? ");
- jdbcClient.executeUpdate(connection, additionalTableDeleteSQL.toString(), deadline, minTime);
+ }
+
+ ttlTables.forEach(tablesToDrop::remove);
+ tablesToDrop.removeIf(it -> !it.matches(".*_\\d{8}"));
+ for (String table : tablesToDrop) {
+ final var dropSql = new SQLBuilder("drop table if exists ").append(table);
+ jdbcClient.executeUpdate(dropSql.toString());
+ }
+
+ long deadline;
+ long minTime;
+ if (model.isRecord()) {
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmmss"));
+ minTime = 1000_00_00_00_00_00L;
+ } else {
+ switch (model.getDownsampling()) {
+ case Minute:
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHHmm"));
+ minTime = 1000_00_00_00_00L;
+ break;
+ case Hour:
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMddHH"));
+ minTime = 1000_00_00_00L;
+ break;
+ case Day:
+ deadline = Long.parseLong(new DateTime().plusDays(-ttl).toString("yyyyMMdd"));
+ minTime = 1000_00_00L;
+ break;
+ default:
+ return;
}
- } catch (JDBCClientException | SQLException e) {
- throw new IOException(e.getMessage(), e);
+ }
+ // Delete data in additional tables
+ for (final var additionalTable : model.getSqlDBModelExtension().getAdditionalTables().values()) {
+ SQLBuilder additionalTableDeleteSQL = new SQLBuilder("delete from " + additionalTable.getName() + " where ")
Review Comment:
Why `delete` rather than `drop`? I think the additional table should follow the timeseries process policy.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1471535138
> > [Disable SS related tests](https://github.com/apache/skywalking/pull/10544/commits/e59626b6a51b8f596bff0379aa270e0b797289e6)
>
> Does this mean we will skip SS implementation for this PR?
Yes, I have no idea (and I have no interest in) supporting ShardingSphere if I can choose, honestly. I'll manage to make the tests passed in a following PR tho.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139823374
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
> It is worth to a cache without loader mechanism to deal with this.
What do you mean by "a cache without loader mechanism"?
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138596144
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
OK, I noticed this is another misguide by naming. This method is from TTL, so, most likely we should rename to `getExistingTables` or similar.
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
Review Comment:
```suggestion
public List<String> getExistingTables(String modelName, long timeBucketStart, long timeBucketEnd) {
```
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139978611
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -125,13 +125,10 @@ public List<String> getTablesForRead(String modelName, long timeBucketStart, lon
timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
- final var tablesWithinTTL = new HashSet<>(getTablesWithinTTL(modelName));
-
return LongStream
.rangeClosed(timeBucketStart, timeBucketEnd)
.distinct()
.mapToObj(it -> tableName + "_" + it)
- .filter(tablesWithinTTL::contains)
Review Comment:
There was a infinite recursive call, just optimized it and add it back.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wankai123 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138322950
##########
oap-server/server-storage-plugin/storage-shardingsphere-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/shardingsphere/ShardingRulesOperator.java:
##########
@@ -157,23 +152,35 @@ private ShardingRule.ShardingRuleBuilder buildShardingRule(ShardingRule.Sharding
buildDataNodes(builder, tableName, dataSources, ttl, currentDate);
buildDatabaseStrategy(builder, dsShardingColumn, dataSources.size());
- switch (shardingAlgorithm) {
- case TIME_SEC_RANGE_SHARDING_ALGORITHM:
- buildTimeRangeTableStrategy(builder, tableShardingColumn, TIME_SEC_RANGE_SHARDING_EXPRESSION);
- break;
- case TIME_MIN_RANGE_SHARDING_ALGORITHM:
- buildTimeRangeTableStrategy(builder, tableShardingColumn, TIME_MIN_RANGE_SHARDING_EXPRESSION);
- break;
- case TIME_RELATIVE_ID_SHARDING_ALGORITHM:
- buildExpressionTableStrategy(builder, tableName, tableShardingColumn,
- TIME_RELATIVE_ID_SHARDING_EXPRESSION);
- break;
- case TIME_BUCKET_SHARDING_ALGORITHM:
- buildExpressionTableStrategy(builder, tableName, tableShardingColumn, TIME_BUCKET_SHARDING_EXPRESSION);
- break;
- default:
- throw new UnexpectedException("Unsupported sharding algorithm " + shardingAlgorithm);
- }
Review Comment:
We don't need these anymore? But I don't see you remove them from the top of the Class code.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1471645191
> > > [Disable SS related tests](https://github.com/apache/skywalking/pull/10544/commits/e59626b6a51b8f596bff0379aa270e0b797289e6)
> >
> >
> > Does this mean we will skip SS implementation for this PR?
>
> Yes, I have no idea (and I have no interest in) supporting ShardingSphere if I can choose, honestly. I'll manage to make the tests passed in a following PR tho.
Then, we should disable all SS e2e for now.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138576867
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
Review Comment:
```suggestion
public static String getLatestTableForWrite(Model model) {
```
I feel this should not be an overload method. It only represents for getting the name of latest table.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1140140810
##########
docs/en/changes/changes.md:
##########
@@ -11,6 +11,7 @@
* Support to bind TLS status as a part of component for service topology.
* Fix component ID priority bug.
* Fix component ID of topology overlap due to storage layer bugs.
+* [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling.
Review Comment:
```suggestion
* [Breaking Change] Enhance JDBC storage through merging tables and managing day-based table rolling.
* [Breaking Change] Sharding-MySQL implementations and tests get removed due to we have the day-based rolling mechanism by default
```
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138290298
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/type/SelectedRecord.java:
##########
@@ -38,7 +38,7 @@ public class SelectedRecord {
/**
* Usually an integer value as this is metrics.
*/
- private String value;
+ private long value;
Review Comment:
I think you missed reverting this back.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139970652
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -125,13 +125,10 @@ public List<String> getTablesForRead(String modelName, long timeBucketStart, lon
timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
- final var tablesWithinTTL = new HashSet<>(getTablesWithinTTL(modelName));
-
return LongStream
.rangeClosed(timeBucketStart, timeBucketEnd)
.distinct()
.mapToObj(it -> tableName + "_" + it)
- .filter(tablesWithinTTL::contains)
Review Comment:
I think this is a good one. Why do you 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #10544:
URL: https://github.com/apache/skywalking/pull/10544
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138585726
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
I think failing to read from Database in sync may be not worth it.
1. It could be slow
2. If this kind of query keeps happening, we cost a lot of resources in the database.
Instead, moving the table cache from `DeleteDao` to JDBC global scope should help. After all, it only read once period.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138591933
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/dao/JDBCHistoryDeleteDAO.java:
##########
@@ -18,66 +18,100 @@
package org.apache.skywalking.oap.server.storage.plugin.jdbc.common.dao;
-import java.io.IOException;
-import java.sql.Connection;
-import java.sql.SQLException;
+import lombok.RequiredArgsConstructor;
+import lombok.SneakyThrows;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
import org.apache.skywalking.oap.server.core.storage.IHistoryDeleteDAO;
import org.apache.skywalking.oap.server.core.storage.model.Model;
-import org.apache.skywalking.oap.server.core.storage.model.SQLDatabaseModelExtension;
-import org.apache.skywalking.oap.server.library.client.jdbc.JDBCClientException;
-import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCHikariCPClient;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
import org.apache.skywalking.oap.server.storage.plugin.jdbc.SQLBuilder;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.JDBCTableInstaller;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.common.TableHelper;
import org.joda.time.DateTime;
-import lombok.RequiredArgsConstructor;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+
+@Slf4j
@RequiredArgsConstructor
public class JDBCHistoryDeleteDAO implements IHistoryDeleteDAO {
- private final JDBCHikariCPClient jdbcClient;
+ private final JDBCClient jdbcClient;
+ private final TableHelper tableHelper;
+ private final JDBCTableInstaller modelInstaller;
+
+ private final Map<Pair<Model, String>, Boolean> tableExistenceCache = new ConcurrentHashMap<>();
@Override
- public void deleteHistory(Model model, String timeBucketColumnName, int ttl) throws IOException {
- SQLBuilder dataDeleteSQL = new SQLBuilder("delete from " + model.getName() + " where ")
- .append(timeBucketColumnName).append("<= ? ")
- .append(" and ")
- .append(timeBucketColumnName).append(">= ? ");
+ @SneakyThrows
+ public void deleteHistory(Model model, String timeBucketColumnName, int ttl) {
+ final var endTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
Review Comment:
`IHistoryDeleteDAO` is driven by DataTTLKeeperTimer for a period of 5 mins. I think we don't need to check the sync at such a high frequency.
Ref `HistoryDeleteEsDAO#indexLatestSuccess`, reduced the load. JDBC was about deleting from tables, so, can't follow 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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1138596601
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
Review Comment:
```suggestion
public List<String> getExistingTables(String modelName, long timeBucketStart, long timeBucketEnd) {
```
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139832503
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.skywalking.oap.server.storage.plugin.jdbc.common;
+
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.FunctionCategory;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.config.ConfigService;
+import org.apache.skywalking.oap.server.core.storage.model.Model;
+import org.apache.skywalking.oap.server.library.client.jdbc.hikaricp.JDBCClient;
+import org.apache.skywalking.oap.server.library.module.ModuleManager;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+import org.apache.skywalking.oap.server.storage.plugin.jdbc.TableMetaInfo;
+
+import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+
+/**
+ * Utility class to get table name for a given model.
+ */
+@Slf4j
+@RequiredArgsConstructor
+public class TableHelper {
+ private final ModuleManager moduleManager;
+ private final JDBCClient jdbcClient;
+
+ public static String getTableName(Model model) {
+ final var aggFuncName = FunctionCategory.uniqueFunctionName(model.getStreamClass()).replaceAll("-", "_");
+ return StringUtil.isNotBlank(aggFuncName) ? aggFuncName : model.getName();
+ }
+
+ public static String getTableForWrite(Model model) {
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ final var dayTimeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ return tableName + Const.UNDERSCORE + dayTimeBucket;
+ }
+
+ public static String getTable(Model model, long timeBucket) {
+ final var tableName = getTableName(model);
+ if (timeBucket == 0) {
+ timeBucket = TimeBucket.getTimeBucket(System.currentTimeMillis(), DownSampling.Day);
+ }
+
+ if (!model.isTimeSeries()) {
+ return tableName;
+ }
+
+ return tableName + Const.UNDERSCORE + TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucket), DownSampling.Day);
+ }
+
+ public List<String> getTablesForRead(String modelName, long timeBucketStart, long timeBucketEnd) {
+ final var model = TableMetaInfo.get(modelName);
+ final var tableName = getTableName(model);
+
+ if (!model.isTimeSeries()) {
+ return Collections.singletonList(tableName);
+ }
+
+ timeBucketStart = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketStart), DownSampling.Day);
+ timeBucketEnd = TimeBucket.getTimeBucket(TimeBucket.getTimestamp(timeBucketEnd), DownSampling.Day);
+
+ return LongStream
+ .rangeClosed(timeBucketStart, timeBucketEnd)
+ .distinct()
+ .mapToObj(it -> tableName + "_" + it)
+ .filter(table -> {
+ try {
+ return jdbcClient.tableExists(table);
Review Comment:
A cache with timeout load mechanism.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139936010
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
Sounds like you want to capture the "table not found SQL exception", if that's the case, we don't even need a loading cache, we just use a plain cache (HashMap), and query the database for the first time, since then we only remove the cache when captured the exception, and the cache won't never expire unless exception is caught. Capturing such exception requires string manipulation though.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139944026
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
> Sounds like you want to capture the "table not found SQL exception", if that's the case, we don't even need a loading cache, we just use a plain cache (HashMap), and query the database for the first time, since then we only remove the cache when captured the exception, and the cache won't never expire unless exception is caught. Capturing such exception requires string manipulation though.
Make sense. Map could be easier to archive this than cache.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139944332
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
The cache structure is `<String table, Boolean exists>`, so if the `N - 7` tables are absent, the results will be cached for 10 minutes (`expireAfterWrite(10, TimeUnit.MINUTES)`), so within the 10 minutes, no query will be sent to database. It's nothing special for the `N - 7` tables actually.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#discussion_r1139968094
##########
oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/TableHelper.java:
##########
@@ -54,6 +58,16 @@ public class TableHelper {
@Getter(lazy = true, value = AccessLevel.PRIVATE)
private final ConfigService configService = moduleManager.find(CoreModule.NAME).provider().getService(ConfigService.class);
+ private final LoadingCache<String, Boolean> tableExistence =
+ CacheBuilder.newBuilder()
+ .expireAfterWrite(10, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ public @NonNull Boolean load(@NonNull String tableName) throws Exception {
+ return jdbcClient.tableExists(tableName);
+ }
+ });
Review Comment:
Let's just change to `expireAfterRead`. I think it is enough.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10544: [Breaking Change] Enhance JDBC storage through merging table and managing day-based table rolling
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10544:
URL: https://github.com/apache/skywalking/pull/10544#issuecomment-1473497170
I think I am mostly done with high-priority things for now.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org