You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by "chenggui53 (via GitHub)" <gi...@apache.org> on 2023/06/08 12:29:07 UTC

[GitHub] [incubator-devlake] chenggui53 opened a new pull request, #5411: refactor: add subtask register for github plugin

chenggui53 opened a new pull request, #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411

   ### ⚠️ Pre Checklist
   
   > Please complete _ALL_ items in this checklist, and remove before submitting
   
   - [ ] I have read through the [Contributing Documentation](https://devlake.apache.org/community/).
   - [ ] I have added relevant tests.
   - [ ] I have added relevant documentation.
   - [ ] I will add labels to the PR, such as `pr-type/bug-fix`, `pr-type/feature-development`, etc.
   
   <!--
   Thanks for submitting a pull request!
   
   We appreciate you spending the time to work on these changes.
   Please fill out as many sections below as possible.
   -->
   
   ### Summary
   What does this PR do?
   like pr https://github.com/apache/incubator-devlake/pull/4896, add subtask register for github
   
   ### Does this close any open issues?
   Closes #4413
   
   ### Screenshots
   Include any relevant screenshots here.
   
   ### Other Information
   Any other information that is important to 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: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1584096808

   Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor.
   I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.
   
   BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1263774024


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,104 @@
+/*
+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 sorter
+
+import (
+	"fmt"
+	"sort"
+
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	// 1. construct tmp data struct
+	subtaskNameMetaMap := make(map[string]*plugin.SubTaskMeta)
+	nameDependencyMap := make(map[string][]string)
+	nameProductMap := make(map[string][]string)
+
+	productList := make([]string, 0)
+	for _, item := range metas {
+		subtaskNameMetaMap[item.Name] = item
+		nameDependencyMap[item.Name] = item.DependencyTables
+
+		// check if subtask product same tables, which may cause cyclic error
+		for _, productItem := range item.ProductTables {
+			if contains(productList, productItem) {
+				return nil, errors.Convert(fmt.Errorf("duplicate product detected %s", productItem))

Review Comment:
   In fact, when I wrote here, I wondered whether there would be a sequence and conflict between different tasks operating on the same table, but unfortunately, I didn't think about this issue carefully and ask you for confirmation in time πŸ˜…



##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,104 @@
+/*
+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 sorter
+
+import (
+	"fmt"
+	"sort"
+
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	// 1. construct tmp data struct
+	subtaskNameMetaMap := make(map[string]*plugin.SubTaskMeta)
+	nameDependencyMap := make(map[string][]string)
+	nameProductMap := make(map[string][]string)
+
+	productList := make([]string, 0)
+	for _, item := range metas {
+		subtaskNameMetaMap[item.Name] = item
+		nameDependencyMap[item.Name] = item.DependencyTables
+
+		// check if subtask product same tables, which may cause cyclic error
+		for _, productItem := range item.ProductTables {
+			if contains(productList, productItem) {
+				return nil, errors.Convert(fmt.Errorf("duplicate product detected %s", productItem))

Review Comment:
   In fact, when I wrote here, I wondered whether there would be a sequence and conflict between different tasks operating on the same table, but unfortunately, I didn't think about this issue carefully and ask you for confirmation in time πŸ˜…



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1267446769


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {
+	// construct map by metas and their produced tables, the key is table, and value is metas
+	tableMetasMap := make(map[string][]*plugin.SubTaskMeta)
+	for _, item := range metas {
+		for _, tableItem := range item.ProductTables {
+			if value, ok := tableMetasMap[tableItem]; ok {
+				tableMetasMap[tableItem] = append(value, item)
+			} else {
+				tableMetasMap[tableItem] = []*plugin.SubTaskMeta{item}
+			}
+		}
+	}
+	// construct meta dependencies by meta.TableDependencies
+	for _, metaItem := range metas {
+		for _, tableItem := range metaItem.DependencyTables {

Review Comment:
   thanks, i will update it later 



##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {
+	// construct map by metas and their produced tables, the key is table, and value is metas
+	tableMetasMap := make(map[string][]*plugin.SubTaskMeta)
+	for _, item := range metas {
+		for _, tableItem := range item.ProductTables {
+			if value, ok := tableMetasMap[tableItem]; ok {
+				tableMetasMap[tableItem] = append(value, item)
+			} else {
+				tableMetasMap[tableItem] = []*plugin.SubTaskMeta{item}
+			}
+		}
+	}
+	// construct meta dependencies by meta.TableDependencies
+	for _, metaItem := range metas {
+		for _, tableItem := range metaItem.DependencyTables {

Review Comment:
   Thanks, I will update later.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1267446769


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {
+	// construct map by metas and their produced tables, the key is table, and value is metas
+	tableMetasMap := make(map[string][]*plugin.SubTaskMeta)
+	for _, item := range metas {
+		for _, tableItem := range item.ProductTables {
+			if value, ok := tableMetasMap[tableItem]; ok {
+				tableMetasMap[tableItem] = append(value, item)
+			} else {
+				tableMetasMap[tableItem] = []*plugin.SubTaskMeta{item}
+			}
+		}
+	}
+	// construct meta dependencies by meta.TableDependencies
+	for _, metaItem := range metas {
+		for _, tableItem := range metaItem.DependencyTables {

Review Comment:
   thanks, i will update it later 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1643414498

   > @chenggui53 Seems ready to merge, am I right?
   
   @klesh Yes, I think so.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "keon94 (via GitHub)" <gi...@apache.org>.
keon94 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1244702000


##########
backend/helpers/pluginhelper/subtaskmeta_sorter/subtask.go:
##########
@@ -60,59 +60,21 @@ func topologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, error)
 		}
 	}
 
-	orderedSubtaskList := make([]plugin.SubTaskMeta, 0)
-	for {
-		if len(dependenciesMap) == 0 {
-			break
-		}
-
-		tmpList := make([]string, 0)
-		for key, item := range dependenciesMap {
-			if len(item) == 0 {
-				tmpList = append(tmpList, key)
-			}
-		}
-		if len(tmpList) == 0 {
-			return nil, fmt.Errorf("cyclic dependency detected: %v", dependenciesMap)
-		}
-
-		// remove item in dependencies map
-		for key, value := range dependenciesMap {
-			if contains(tmpList, key) {
-				delete(dependenciesMap, key)
-			} else {
-				dependenciesMap[key] = removeElements(value, tmpList)
-			}
-		}
-
-		sort.Strings(tmpList)
-		// convert item to subtaskmeta by name, and append to orderedSubtaskList
-		for _, item := range tmpList {
-			value, ok := nameMetaMap[item]
-			if !ok {
-				return nil, fmt.Errorf("illeagal subtaskmeta detected %s", item)
-			}
-			orderedSubtaskList = append(orderedSubtaskList, *value)
-		}
+	// sort
+	orderStrList, err := topologicalSortSameElements(dependenciesMap)
+	if err != nil {
+		return nil, err
 	}
-	return orderedSubtaskList, nil
-}
 
-func contains[T comparable](itemList []T, item T) bool {
-	for _, newItem := range itemList {
-		if item == newItem {
-			return true
+	// gen list by sorted name list and return
+	orderedSubtaskList := make([]plugin.SubTaskMeta, 0)
+	for _, item := range orderStrList {
+		value, ok := nameMetaMap[item]
+		if !ok {
+			return nil, fmt.Errorf("illeagal subtaskmeta detected %s", item)

Review Comment:
   typo: "illegal"



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1610727388

   
   
   
   > I tried to draw a DAG to depict how the data flow might look, I hope that gives you the idea or the problem, feel free to ask me anything
   > 
   > ![2023-06-28 2023-06-28 11 34 38 excalidraw](https://user-images.githubusercontent.com/61080/249343697-f6fde36d-3d70-4434-8232-fca8052986c8.svg)
   
   This graph is very helpful, I will improve it ASAP πŸ˜€


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1610653437

   I tried to draw a  DAG to depict how the data flow might look, I hope that gives you the idea or the problem, feel free to ask me anything
   
   ![2023-06-28 2023-06-28 11 34 38 excalidraw](https://github.com/apache/incubator-devlake/assets/61080/f6fde36d-3d70-4434-8232-fca8052986c8)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1633953277

   > Hi @klesh , something is not clear for me, can you help to check why github.repo_converter set RAW_REPOSITORIES_TABLE in RawDataSubTaskArgs.Table, and set GithubRepo input for Convert function. I only see RAW_REPOSITORIES_TABLE used once in here, and can't figure out the releation between RAW_REPOSITORIES_TABLE and GithubRepo.
   
   Aah, good question, we used to collect repo information from the API and store it to the raw data table.
   However, this behavior has been deprecated recently and users would create the tool layer record by calling this API https://github.com/apache/incubator-devlake/blob/990c19ded0f6e23f60aa05cd15bb6bc3e9e66c95/backend/plugins/github/impl/impl.go#L234
   
   In general, no more raw tables for scopes (GitHub repo, jira board, etc.)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1264810146


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,104 @@
+/*
+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 sorter
+
+import (
+	"fmt"
+	"sort"
+
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	// 1. construct tmp data struct
+	subtaskNameMetaMap := make(map[string]*plugin.SubTaskMeta)
+	nameDependencyMap := make(map[string][]string)
+	nameProductMap := make(map[string][]string)
+
+	productList := make([]string, 0)
+	for _, item := range metas {
+		subtaskNameMetaMap[item.Name] = item
+		nameDependencyMap[item.Name] = item.DependencyTables
+
+		// check if subtask product same tables, which may cause cyclic error
+		for _, productItem := range item.ProductTables {
+			if contains(productList, productItem) {
+				return nil, errors.Convert(fmt.Errorf("duplicate product detected %s", productItem))

Review Comment:
   No problem, it happens.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1609572620

   > hi @klesh , I've done a version, can you help to review it when you have time :) here are the changes i've done:
   > 
   > 1. Add table-based dependency registration and sorting in plugin helper
   > 2. Add register and table dependencies in github plugin
   > 3. To avoid changes in the gitlab plugin, I've kept subtaskMeta dependencies code and i will update it in another new pr.
   > 
   > Some restrictions are also introduced
   > 
   > 1. If a subtask task category depends on another task to handle this table, then an temporary table dependency should be added to the dependency of this task to ensure that a single data table is processed before that, which I think is also very understandable to developers.
   >    e.g. like EnrichPullRequestIssuesMeta and ConvertPullRequestIssuesMeta in github plugin, they depends on PullReqeust task to process data in RAW_PULL_REQUEST_TABLE, so i added tool_layer in their table dependency
   > 
   > With that changes, we can process subtask concurrency (like we discussed in pr #4896), I can submit a new PR after completing the subtask register changes to support concurrent execution of subtasks in plugins.
   
   OMG, you have finished the table dependency sorting? way to go, man.
   Will look ASAP, however, I'm kind of hands full at the moment.
   @keon94 @CamilleTeruel Please take a look at it as well. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1267439493


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {
+	// construct map by metas and their produced tables, the key is table, and value is metas
+	tableMetasMap := make(map[string][]*plugin.SubTaskMeta)
+	for _, item := range metas {
+		for _, tableItem := range item.ProductTables {
+			if value, ok := tableMetasMap[tableItem]; ok {
+				tableMetasMap[tableItem] = append(value, item)
+			} else {
+				tableMetasMap[tableItem] = []*plugin.SubTaskMeta{item}
+			}
+		}
+	}
+	// construct meta dependencies by meta.TableDependencies
+	for _, metaItem := range metas {
+		for _, tableItem := range metaItem.DependencyTables {

Review Comment:
   It would be better to create a temporary `map` for each `metaItem` and reuse it for deduplication instead of creating one on each `append`
   



##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table_test.go:
##########
@@ -0,0 +1,81 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+	"reflect"
+	"testing"
+)
+
+func Test_tableTopologicalSort(t *testing.T) {
+	pluginA := plugin.SubTaskMeta{
+		Name:             "A",
+		DependencyTables: []string{},
+		ProductTables:    []string{"_TOOL_TEST_TABLE"},

Review Comment:
   It would be better to simulate the situation I described in the previous review that multiple subtasks might produce records for the same table



##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {

Review Comment:
   I believe so. Good job.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "keon94 (via GitHub)" <gi...@apache.org>.
keon94 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1244701515


##########
backend/helpers/pluginhelper/subtaskmeta_sorter/subtask.go:
##########
@@ -32,11 +32,11 @@ func NewDependencySorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
 }
 
 func (d *DependencySorter) Sort() ([]plugin.SubTaskMeta, error) {
-	return topologicalSort(d.metas)
+	return dependenciesTopologicalSort(d.metas)
 }
 
 // stable topological sort
-func topologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, error) {

Review Comment:
   Our convention is to use errors.Error (our own impl) in our code instead of the base Go error interface



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1614052824

   > > > Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor. I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.
   > > > BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?
   > > 
   > > 
   > > @klesh Your comments are very helpful, this register is a step forward, I will fix this part as soon as possible. And thank you very much for your invitation, I am very happy to be a committer of this project
   > 
   > Nice, I will nominate you ASAP, it may take a couple of weeks.
   
   Hi @klesh , I haven't received any relevant emails or messages until now. In order to avoid any notifications that I missed, I would like to ask if this is normal. 😁


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1617180933

   > > > > Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor. I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.
   > > > > BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?
   > > > 
   > > > 
   > > > @klesh Your comments are very helpful, this register is a step forward, I will fix this part as soon as possible. And thank you very much for your invitation, I am very happy to be a committer of this project
   > > 
   > > 
   > > Nice, I will nominate you ASAP, it may take a couple of weeks.
   > 
   > Hi @klesh , I haven't received any relevant emails or messages until now. In order to avoid any notifications that I missed, I would like to ask if this is normal. 😁
   
    Yes, there is one ongoing vote in the queue, we have to wait for that to avoid confusing our mentors. Will be voting for you when it closed.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1267430954


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {

Review Comment:
   Hi @klesh , does this function match what you mentioned earlier?



##########
backend/plugins/github/tasks/commit_convertor.go:
##########
@@ -64,7 +77,7 @@ func ConvertCommits(taskCtx plugin.SubTaskContext) errors.Error {
 				ConnectionId: data.Options.ConnectionId,
 				Name:         data.Options.Name,
 			},
-			Table: RAW_COMMENTS_TABLE,
+			Table: RAW_COMMIT_TABLE,

Review Comment:
   Hi @klesh , i'm not sure if it's a typo here, commit converter uses a raw_comments_talbe instead of a raw_commit_table. Can you help to check this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1584466424

   > > Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor. I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.
   > > BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?
   > 
   > @klesh Your comments are very helpful, this register is a step forward, I will fix this part as soon as possible. And thank you very much for your invitation, I am very happy to be a committer of this project
   
   Nice, I will nominate you ASAP, it may take a couple of weeks.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1266694660


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {

Review Comment:
   Hi @klesh , I'm not sure if this is what you mentioned before. Can you help review it. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1608816069

   hi @klesh , I've done a version, can you help to review it when you have time :)
   here are the changes i've done:
   1. Add table-based dependency registration and sorting in plugin helper
   2. Add register and table dependencies in github plugin
   3. To avoid changes in the gitlab plugin, I've kept subtaskMeta dependencies code and i will update it in another new pr.
   
   Some restrictions are also introduced
   1. If a subtask task category depends on another task to handle this table, then an temporary table dependency should be added to the dependency of this task to ensure that a single data table is processed before that, which I think is also very understandable to developers.
   e.g. like EnrichPullRequestIssuesMeta and ConvertPullRequestIssuesMeta in github plugin, they depends on PullReqeust task to process data in RAW_PULL_REQUEST_TABLE, so i added tool_layer in their table dependency
   
   With that changes, we can process subtask concurrency (like we discussed in pr #4896),  I can submit a new PR after completing the subtask register changes to support concurrent execution of subtasks in plugins.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1609625615

   > Not sure if I understand it correctly, seems like subtasks are declaring only the dependency , how does it work? I believe we need both **dependency** and **product** tables to connect those **subtasks** correctly.
   
   It seems that I have a misunderstanding here. Let me have a look again πŸ˜…


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1633916248

   Hi @klesh , something is not clear for me, can you help to check why github.repo_converter set RAW_REPOSITORIES_TABLE  in RawDataSubTaskArgs.Table, and set GithubRepo input for Convert function. I only see RAW_REPOSITORIES_TABLE used once in here,  and can't figure out the releation between RAW_REPOSITORIES_TABLE and GithubRepo.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1267431648


##########
backend/plugins/github/tasks/commit_convertor.go:
##########
@@ -64,7 +77,7 @@ func ConvertCommits(taskCtx plugin.SubTaskContext) errors.Error {
 				ConnectionId: data.Options.ConnectionId,
 				Name:         data.Options.Name,
 			},
-			Table: RAW_COMMENTS_TABLE,
+			Table: RAW_COMMIT_TABLE,

Review Comment:
   You are correct, definitely a typo for sure.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1266694660


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,74 @@
+/*
+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 sorter
+
+import (
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	constructedMetas := constructDependenciesByTable(metas)
+	return dependenciesTopologicalSort(constructedMetas)
+}
+
+func constructDependenciesByTable(metas []*plugin.SubTaskMeta) []*plugin.SubTaskMeta {

Review Comment:
   Hi @klesh , I'm not sure if this is what you mentioned before. Can you help review it. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1583814458

   Hi @klesh, Should i create pr for each plugin to add this register, or do this in one pr ?  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1223931990


##########
backend/plugins/github/tasks/pr_commit_collector.go:
##########
@@ -44,6 +48,7 @@ var CollectApiPullRequestCommitsMeta = plugin.SubTaskMeta{
 	EnabledByDefault: true,
 	Description:      "Collect PullRequestCommits data from Github api, supports both timeFilter and diffSync.",
 	DomainTypes:      []string{plugin.DOMAIN_TYPE_CROSS, plugin.DOMAIN_TYPE_CODE_REVIEW},
+	Dependencies:     []*plugin.SubTaskMeta{&ExtractApiEventsMeta},

Review Comment:
   I just realize that the `Dependencies` are defined based on the current order which would work but is not accurate.
   For example, the `CollectApiPullRequestCommitsMeta` should depend on the `ExtractApiPullRequestMete` instead of `ExtractApiEventsMeta`.
   Would you mind updating the code to reflect that? The trick is to identify [which table](https://github.com/apache/incubator-devlake/pull/5411/files#diff-124f6bf67e82ddaafdc0fb6d16ab9ed8afa26c0937a25b259046520ebe1ada4dR79) is required in the collector as the `Input` parameter, and find out the Extractor of the 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: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1223722632


##########
backend/plugins/github/impl/impl.go:
##########
@@ -95,53 +102,11 @@ func (p Github) Description() string {
 }
 
 func (p Github) SubTaskMetas() []plugin.SubTaskMeta {
-	return []plugin.SubTaskMeta{
-		tasks.CollectApiIssuesMeta,
-		tasks.ExtractApiIssuesMeta,
-		tasks.CollectApiPullRequestsMeta,
-		tasks.ExtractApiPullRequestsMeta,
-		tasks.CollectApiCommentsMeta,
-		tasks.ExtractApiCommentsMeta,
-		tasks.CollectApiEventsMeta,
-		tasks.ExtractApiEventsMeta,
-		tasks.CollectApiPullRequestCommitsMeta,
-		tasks.ExtractApiPullRequestCommitsMeta,
-		tasks.CollectApiPullRequestReviewsMeta,
-		tasks.ExtractApiPullRequestReviewsMeta,
-		tasks.CollectApiPrReviewCommentsMeta,
-		tasks.ExtractApiPrReviewCommentsMeta,
-		tasks.CollectApiCommitsMeta,
-		tasks.ExtractApiCommitsMeta,
-		tasks.CollectApiCommitStatsMeta,
-		tasks.ExtractApiCommitStatsMeta,
-		tasks.CollectMilestonesMeta,
-		tasks.ExtractMilestonesMeta,
-		tasks.CollectAccountsMeta,
-		tasks.ExtractAccountsMeta,
-		tasks.CollectAccountOrgMeta,
-		tasks.ExtractAccountOrgMeta,
-		tasks.CollectRunsMeta,
-		tasks.ExtractRunsMeta,
-		tasks.ConvertRunsMeta,
-		tasks.CollectJobsMeta,
-		tasks.ExtractJobsMeta,
-		tasks.ConvertJobsMeta,
-		tasks.EnrichPullRequestIssuesMeta,
-		tasks.ConvertRepoMeta,
-		tasks.ConvertIssuesMeta,
-		tasks.ConvertIssueAssigneeMeta,
-		tasks.ConvertCommitsMeta,
-		tasks.ConvertIssueLabelsMeta,
-		tasks.ConvertPullRequestCommitsMeta,
-		tasks.ConvertPullRequestsMeta,
-		tasks.ConvertPullRequestReviewsMeta,
-		tasks.ConvertPullRequestLabelsMeta,
-		tasks.ConvertPullRequestIssuesMeta,
-		tasks.ConvertIssueCommentsMeta,
-		tasks.ConvertPullRequestCommentsMeta,
-		tasks.ConvertMilestonesMeta,
-		tasks.ConvertAccountsMeta,
+	list, err := subtaskmeta_sorter.NewDependencySorter(tasks.SubTaskMetaList).Sort()

Review Comment:
   Can we reuse the result from https://github.com/apache/incubator-devlake/pull/5411/files#diff-5d9d2b5b08a579490b7eeaba1746905ca45123a978ec531bd584183eec384fbfR51 instead of sorting it twice?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1592527732

   > @chenggui53 I thought we agree on "One PR for Plugin" so I can help verify the dependencies are correct. Seems like this PR contains multiple plugins.  
   
   @klesh sorry, i've modified multiple plugins in this branch for test，it's not ready for review. i will create a new branch for test and request your review when it 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: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1592519054

   @chenggui53 I thought we agree on "One PR for Plugin" so I can help verify the dependencies are correct. Seems like this PR contains multiple plugins.  


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 closed pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 closed pull request #5411: refactor: add subtask register for github plugin
URL: https://github.com/apache/incubator-devlake/pull/5411


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh merged pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh merged PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on a diff in pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on code in PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#discussion_r1263443688


##########
backend/helpers/pluginhelper/subtaskmeta/sorter/table.go:
##########
@@ -0,0 +1,104 @@
+/*
+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 sorter
+
+import (
+	"fmt"
+	"sort"
+
+	"github.com/apache/incubator-devlake/core/errors"
+	"github.com/apache/incubator-devlake/core/plugin"
+)
+
+type TableSorter struct {
+	metas []*plugin.SubTaskMeta
+}
+
+func NewTableSorter(metas []*plugin.SubTaskMeta) SubTaskMetaSorter {
+	return &TableSorter{metas: metas}
+}
+
+func (d *TableSorter) Sort() ([]plugin.SubTaskMeta, errors.Error) {
+	return tableTopologicalSort(d.metas)
+}
+
+func tableTopologicalSort(metas []*plugin.SubTaskMeta) ([]plugin.SubTaskMeta, errors.Error) {
+	// 1. construct tmp data struct
+	subtaskNameMetaMap := make(map[string]*plugin.SubTaskMeta)
+	nameDependencyMap := make(map[string][]string)
+	nameProductMap := make(map[string][]string)
+
+	productList := make([]string, 0)
+	for _, item := range metas {
+		subtaskNameMetaMap[item.Name] = item
+		nameDependencyMap[item.Name] = item.DependencyTables
+
+		// check if subtask product same tables, which may cause cyclic error
+		for _, productItem := range item.ProductTables {
+			if contains(productList, productItem) {
+				return nil, errors.Convert(fmt.Errorf("duplicate product detected %s", productItem))

Review Comment:
   It doesn't seem right because **records in a table may come from multiple subtasks**, for example
   the jira issue extractor [produces `Changelog`](https://github.com/apache/incubator-devlake/blob/927db42404f393557b26982761c70b06f462d0da/backend/plugins/jira/tasks/issue_extractor.go#L154) while [changelog collector](https://github.com/apache/incubator-devlake/blob/15afa46cf82ce479fb46757734636cf787faff69/backend/plugins/jira/tasks/issue_changelog_collector.go) + [changelog extractor](https://github.com/apache/incubator-devlake/blob/2d08ffaedc9debcb0be367bb84fa18592bb06cec/backend/plugins/jira/tasks/issue_changelog_extractor.go) also producing records to the same table due to Performance Consideration: If we could collect all changelogs from the issues, then we don't need to collect them from the Changelog API which is very very slow.
   
   Maybe we could just deduce and update the `Dependencies` for `SubtaskMeta` based on `ProductTables` and `DependencyTables` and reuse the existing `SubTaskMetaSorter`. 
   1. TableSort does nothing more than figure out what `Dependencies` should be
   2. Sorting and Cyclic Detection can be done in `SubTaskMetaSorter` and one place only



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] klesh commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1643410107

   @chenggui53 Seems ready to merge, am I right?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-devlake] chenggui53 commented on pull request #5411: refactor: add subtask register for github plugin

Posted by "chenggui53 (via GitHub)" <gi...@apache.org>.
chenggui53 commented on PR #5411:
URL: https://github.com/apache/incubator-devlake/pull/5411#issuecomment-1584105601

   > Hi, @chenggui53 , thanks for asking. I think both are fine for this refactor. I saw the dependencies are deduced from the Hardcoded Order which is not accurate, would you mind fixing them as well? Maybe we should do it One by One so I can verify the dependency for you.
   > 
   > BTW, Your contribution means a lot to the project, would you like to join us as an Apache Committer?
   
   Your comments are very helpful, this register is a step forward, I will fix this part as soon as possible. 
   And thank you very much for your invitation, I am very happy to be a committer of this project


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org