Skip to content

[cherry-pick][6.10][CDAP-21269] Add support for Dataproc Flexible Machine Types in compute profiles - #16210

Closed
123-komal wants to merge 684 commits into
release/6.10from
flexvm-support
Closed

[cherry-pick][6.10][CDAP-21269] Add support for Dataproc Flexible Machine Types in compute profiles#16210
123-komal wants to merge 684 commits into
release/6.10from
flexvm-support

Conversation

@123-komal

Copy link
Copy Markdown
Contributor

Original PR: #16204

sidhdirenge and others added 30 commits February 5, 2025 16:13
[CDAP-21131] Delay in spanner table creation should not crash stateless pods
[CDAP-21096] Separate bind and announce port configurations for appfabric server and processor for startupProbe
[CDAP-21128] Add rule based error classification
[CDAP-21123] Avoid double wrapping of failure details provider during starting state
[CDAP-21115] Fix method used to base64 encode basic auth header
[CDAP-21096] Use RemoteScheduleManager to update TimeSchedulerService from Appfabric service
Adding namespace tag to Flow Control metrics
[CDAP-21027] upgrade hadoop to 3.3.6
[CDAP-21128] Handle dataproc job failures in error management
[CDAP-21135] Add pagination in apps list call made during pre-upgrade job
sahusanket and others added 28 commits May 22, 2026 18:10
…yment

CDAP-21248 : Updating the header name to X-CDAP-App-Deployment-Skipped
[CDAP-21245] Fix: Propagate parent logging context to asynchronous action and fork threads
…tation

CDAP-21248 : Make run_records clean up via executePartitionedUpdate for Spanner
# Conflicts:
#	cdap-common/src/main/resources/cdap-default.xml
CDAP-21249 : Add optimistic locking with default false.
Prevent dynamic service deletion during upgrade
add property to configure the limit connection browse calls from wrangler
to remove potential race condition.

In the old implementation, the internal service channel
might get closed from the event thread when send messaging
is happening from the client channel thread.
Fix the autoRead change in the HttpRequestRouter.
…provider_delete

CDAP-21256 : Support preserving client credentials when deleting an OAuth provider
…ancel

CDAP-21219 : Handle force killing remote job more gracefully to avoid race condition
Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated

Updated
[CDAP-21257]Capturing  Dataproc operations
Update cdap-ui submodule to latest develop
 Fix Dataproc LRO metric emission logic for cluster deletion and corrected the  metric name
CDAP-21265 : adding support for TTL in adding secret.
 Add support for Dataproc Flexible Machine Types in compute profiles

 Adding Support for the Flexvm

 Updated

 Updated

 Updated

 Updated

 Updated

 Updated

 Updated

 Updated

 Updated

 Updated

 Updated
@123-komal 123-komal closed this Sep 1, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive error classification framework, including new error categories, types, and utility classes, while refactoring program lifecycle, runtime, and scheduling handlers. It also replaces the run record monitor with a new flow control service, enhances preview runner security, and adds support for skipping duplicate application deployments. Feedback on these changes highlights several areas for improvement: in ErrorCategory, redundant null checks on the final errorCategory field should be removed and block comments converted to Javadoc; in ErrorUtils, magic numbers for HTTP status codes should be replaced with named constants and a Javadoc typo corrected; in FailureDetailsProvider, the Javadoc return type for getErrorCategory needs correction; and in OperationHttpHandler, the regex for parsing filter key-value pairs should be made more flexible.


private final OperationLifecycleManager operationLifecycleManager;
private final int batchSize;
private static final Pattern KEY_VALUE_PATTERN = Pattern.compile("(\"?)(\\w+)=(\\w+)(\"?)");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The regex ("?)(\w+)=(\w+)("?) used for parsing filter key-value pairs is quite restrictive as \w+ only matches alphanumeric characters and underscores. This might be too limiting if filter values need to support other characters in the future (e.g., hyphens). Consider using a more flexible regex to make the filter more robust, for example by using [^=]+ for the value part.

Comment on lines +54 to +56
/*
* Returns the category of the error.
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This comment block uses /* ... */ which is a block comment. For Javadoc, it should be /** ... */. This also applies to the comment for toString() on line 78.

Suggested change
/*
* Returns the category of the error.
*/
/**
* Returns the category of the error.
*/

* such as plugin, provisioning, etc.
* If the category or reason is not known - it will be marked as ‘Others’.
*
* @return a {@String} representing the error category.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The Javadoc for getErrorCategory states that it returns a String, but the method signature indicates it returns an ErrorCategory object. The Javadoc should be corrected to reflect the actual return type.

Suggested change
* @return a {@String} representing the error category.
* @return an {@link ErrorCategory} representing the error category.

* @return A ActionErrorPair object with corrective action and error type.
*/
public static ActionErrorPair getActionErrorByStatusCode(Integer statusCode) {
switch (statusCode) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The switch statement uses several magic numbers for HTTP status codes (e.g., 429, 416, 417). It's a best practice to define these as named constants to improve readability and maintainability. While some of these might not be in HttpURLConnection, they could be defined as private static final fields within this class.

* on the given status code.
*
* @param statusCode The HTTP status code.
* @return A ActionErrorPair object with corrective action and error type.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a typo in the Javadoc. It should be 'an ActionErrorPair' instead of 'A ActionErrorPair'.

Suggested change
* @return A ActionErrorPair object with corrective action and error type.
* @return an ActionErrorPair object with corrective action and error type.

Comment on lines +89 to +91
public ErrorCategoryEnum getParentCategory() {
return errorCategory == null ? ErrorCategoryEnum.OTHERS : errorCategory;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The errorCategory field is final and initialized in all constructors with a non-null value. Therefore, the null check errorCategory == null is redundant and can be removed.

Suggested change
public ErrorCategoryEnum getParentCategory() {
return errorCategory == null ? ErrorCategoryEnum.OTHERS : errorCategory;
}
public ErrorCategoryEnum getParentCategory() {
return errorCategory;
}

Comment on lines +57 to +60
public String getErrorCategory() {
return errorCategory == null ? ErrorCategoryEnum.OTHERS.toString() : subCategory == null
? errorCategory.toString() : String.format("%s-'%s'", errorCategory, subCategory);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The errorCategory field is final and initialized in all constructors with a non-null value. Therefore, the null check errorCategory == null is redundant and can be removed to simplify the code.

Suggested change
public String getErrorCategory() {
return errorCategory == null ? ErrorCategoryEnum.OTHERS.toString() : subCategory == null
? errorCategory.toString() : String.format("%s-'%s'", errorCategory, subCategory);
}
public String getErrorCategory() {
if (subCategory == null) {
return errorCategory.toString();
}
return String.format("%s-'%s'", errorCategory, subCategory);
}

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.