Skip to content
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

HPCC-26634 Check query status only on QueriesOnly roxies #18073

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

wangkx
Copy link
Member

@wangkx wangkx commented Nov 22, 2023

For containerized, the existing WUListQueries tries to check suspended queries for both queriesOnly roxie and non-queriesOnly roxie. the existing WUListQueries returns a query not found error if there is a non-queriesOnly roxie. For bare metal, the existing WUListQueries tries to get suspended queries for every target which contains a RoxieCluster process. The same error occurs for the target which has both RoxieCluster process and ThorCluster process (ex. thor_roxie). The WUListQueries should not check suspended queries for the non-queriesOnly roxie and the target which has both RoxieCluster process and ThorCluster process.

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

Copy link

@wangkx wangkx requested a review from afishbeck November 22, 2023 20:16
Copy link
Member

@afishbeck afishbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangkx a couple comments. Basically I wonder if you want the targets that "canPublishQueries" instead of the ones that "onlyPublishQuries".

Cloud only supports one or the other now, but like bare-metal in may support both in the future.

Perhaps see clusterInfo.canPublishQueries() to understand what I mean.

@@ -689,6 +689,17 @@ extern TPWRAPPER_API void initContainerRoxieTargets(MapStringToMyClass<ISmartSoc
}
}

extern TPWRAPPER_API void queryQueriesOnlyRoxieTargetNames(StringArray& names)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably name this "get" rather than "query" since you are building a list.

But I wonder whether you really want "QueriesOnlyRoxie". Current cloud makes it seem like there are only 2 types but remember in bare-metal there are three types, and it may become like that again in cloud some day.

enum RoxieTargetType
{
RTTUnknown = 0,
RTTPublished = 1,
RTTQueued = 2,
RTTBoth = 3
};
I think RTTPublished means the same as "QueriesOnly" and "RTTQueued" is like "roxie-workunit.

But RTTBoth can also have queries on it.

What it boils down to for cloud is just the function name, but for bare-metal we should probably support RTTBoth and RTTPublished.

This might be too long but the idea would be a function name like "getRoxieTargetsSupportingPublishedQueries()".

{
IConstWUClusterInfo& clusterInfo = clusters.item(i);
if (clusterInfo.getPlatform() == RoxieCluster)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If as mentioned above you change the name to something like "getRoxieTargetsSupportingPublishedQueries()" you may want to check clusterInfo.canPublishQueries() before adding to list?

For containerized, the existing WUListQueries tries to check
suspended queries for all roxies. The existing WUListQueries
returns a query not found error if there is a non-queries roxie.
For bare metal, the existing WUListQueries tries to get suspended
queries for every target which contains a RoxieCluster process.
The same error occurs for the targets which cannot publish queries.
The WUListQueries should not check suspended queries for the
roxies which cannot publish queries.

Signed-off-by: wangkx <[email protected]>
@wangkx
Copy link
Member Author

wangkx commented Dec 15, 2023

@afishbeck I made the changes based on your comments. Please review.

@wangkx wangkx requested a review from afishbeck December 15, 2023 14:31
Copy link
Member

@afishbeck afishbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangkx looks good.

@wangkx
Copy link
Member Author

wangkx commented Dec 15, 2023

@ghalliday this PR is ready to be merged.

@ghalliday ghalliday merged commit 8725e6a into hpcc-systems:candidate-9.4.x Dec 19, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants