-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enhancement] active mv automatically #32829
[Enhancement] active mv automatically #32829
Conversation
ConnectContext.remove(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some suggested improvements:
-
Exception handling:
In several places, the code simply logs the exception with a warning and continues to run. This can make debugging harder since the cause of the problem may get lost. You should consider whether it's appropriate to deal better with exceptions. -
Excess object creation:
Inside the methodtryToActivate(MaterializedView mv)
, you create a newTableName
object only to convert it to a string. If thetoString()
function isn't costly in terms of resources, this might not be an issue, but if it is, consider using another method to construct the full table name fromdbName
andmv.getName()
. -
Running through databases and tables:
You might want to consider more effective data structures or methods for searching through your databases and tables. Currently, the complexity is proportional to the number of databases multiplied by the number of tables, which will lead to performance issues as these numbers increase. -
Inconsistent usage of
Optional
:
IntryToActivate(MaterializedView mv)
, you’re accessingdbName
viaOptional
. But in other parts of the code, null check isn't done explicitly nor withOptional
. Having consistent use ofOptional
is good practice to avoid NullPointerException. -
Thread Safety:
It's unclear whether the code is running in a multithreaded environment or not. If it does, you need to ensure thread safety especially when manipulating shared resources. Consider potential concurrency issues for methods likeprocess()
,runAfterCatalogReady()
, andtryToActivate()
. -
Code comments:
Although there are relevant comments and it's always beneficial to comment your code, remember that the code itself should be mostly self-explanatory. Aim to write descriptive variable, method, and class names so that comments are needed less for explaining what happens and more for why certain decisions were made.
Please ensure to consider these suggestions taking into account the entire application context since some may not be applicable depending on intended behavior of the software, environment among other factors.
@Mergifyio rebase |
✅ Branch has been successfully rebased |
3b9b365
to
b13480c
Compare
private void process() { | ||
Collection<Database> dbs = GlobalStateMgr.getCurrentState().getIdToDb().values(); | ||
for (Database db : CollectionUtils.emptyIfNull(dbs)) { | ||
for (Table table : CollectionUtils.emptyIfNull(db.getTables())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will happen if dropping the db during processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the table list is got from a
ConcurrentHashMap
tryToActivate
will verify the existence of database again, throw exception if database doesn't exist
can we add a logic to active mv during mv refresh? |
Signed-off-by: Murphy <[email protected]>
Signed-off-by: Murphy <[email protected]>
Signed-off-by: Murphy <[email protected]>
b13480c
to
98aeef2
Compare
Signed-off-by: Murphy <[email protected]>
Kudos, SonarCloud Quality Gate passed! |
[BE Incremental Coverage Report]😍 pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]😍 pass : 44 / 51 (86.27%) file detail
|
@Mergifyio backport branch-3.2 |
@Mergifyio backport branch-3.1 |
✅ Backports have been created
|
@Mergifyio backport branch-3.0 |
✅ Backports have been created
|
✅ Backports have been created
|
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e)
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/alter/AlterJobMgr.java # fe/fe-core/src/main/java/com/starrocks/server/GlobalStateMgr.java # fe/fe-core/src/test/java/com/starrocks/analysis/AlterMaterializedViewTest.java
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/alter/AlterJobMgr.java # fe/fe-core/src/main/java/com/starrocks/common/Config.java # fe/fe-core/src/main/java/com/starrocks/server/GlobalStateMgr.java # fe/fe-core/src/test/java/com/starrocks/analysis/AlterMaterializedViewTest.java # fe/fe-core/src/test/java/com/starrocks/scheduler/PartitionBasedMvRefreshProcessorTest.java
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e)
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/alter/AlterJobMgr.java # fe/fe-core/src/main/java/com/starrocks/server/GlobalStateMgr.java # fe/fe-core/src/test/java/com/starrocks/analysis/AlterMaterializedViewTest.java
@Mergifyio backport branch-2.5 |
✅ Backports have been created
|
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/alter/Alter.java # fe/fe-core/src/main/java/com/starrocks/common/Config.java # fe/fe-core/src/main/java/com/starrocks/server/GlobalStateMgr.java # fe/fe-core/src/test/java/com/starrocks/analysis/AlterMaterializedViewTest.java # fe/fe-core/src/test/java/com/starrocks/scheduler/PartitionBasedMvRefreshProcessorTest.java
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e)
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e) Signed-off-by: Murphy <[email protected]>
Signed-off-by: Murphy <[email protected]> (cherry picked from commit c1e3a1e) Signed-off-by: Murphy <[email protected]>
Signed-off-by: Murphy <[email protected]> Co-authored-by: Murphy <[email protected]> Co-authored-by: Murphy <[email protected]>
Signed-off-by: Murphy <[email protected]> Co-authored-by: Murphy <[email protected]> Co-authored-by: Murphy <[email protected]>
Fixes #issue
Introduce a daemon thread
MVActiveChecker
:admin set frontend config('mv_active_checker_interval_seconds'='300')
Use case:
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: