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.

Sign up for GitHub

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

Jump to bottom

Add scan operation runs by status method #15424

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Add scan operation runs by status method #15424

merged 1 commit into from
Nov 14, 2023

Conversation

samdgupi
Copy link
Contributor

  • Added a new store method to fetch operation runs by status
  • Added builder pattern to operation meta
  • Fixed some issues with current store methods
  • Added a operation test base class as some of this functions will be shared with other tests later

@samdgupi samdgupi requested a review from albertshau November 10, 2023 18:44
@samdgupi samdgupi self-assigned this Nov 10, 2023
@samdgupi samdgupi added build Triggers github actions build 6.10 labels Nov 10, 2023
samdgupi
samdgupi commented Nov 10, 2023
View reviewed changes
cdap-app-fabric/src/main/java/io/cdap/cdap/internal/operation/OperationLifecycleManager.java Outdated
/**
* Scan all pending operations. Needed for try running all pending operation during startup.
*
* @param txBatchSize batch size of transaction
Copy link
Contributor Author

@samdgupi samdgupi Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

will remove the unused param

@samdgupi samdgupi requested a review from GnsP November 10, 2023 18:47
albertshau
albertshau reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@albertshau albertshau left a comment

Choose a reason for hiding this comment

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

minor comments

cdap-proto/src/main/java/io/cdap/cdap/proto/operation/OperationMeta.java Outdated
@@ -41,7 +42,7 @@ public class OperationMeta {
* @param createTime timestamp when the operation was created
* @param endTime timestamp when the operation reached an end state
*/
public OperationMeta(Set<OperationResource> resources, Instant createTime, @Nullable Instant endTime) {
private OperationMeta(Set<OperationResource> resources, Instant createTime, @Nullable Instant endTime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cdap-proto/src/main/java/io/cdap/cdap/proto/operation/OperationMeta.java Outdated
}

public Builder setResources(Set<OperationResource> resources) {
this.resources = resources;
Copy link
Contributor

Choose a reason for hiding this comment

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

safer to be defensive here and clear the existing resources, then add all the input (would need to change the constructor to create a modifiable Set)

Copy link
Contributor

Choose a reason for hiding this comment

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

the existing code should also be returning an UnmodifiableSet for getResources()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cdap-proto/src/main/java/io/cdap/cdap/proto/operation/OperationMeta.java Outdated
* @param meta existing meta to copy fields from
*/
public static Builder builder(OperationMeta meta) {
return new Builder(meta);
Copy link
Contributor

Choose a reason for hiding this comment

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

can create the new builder and call the .setXYZ() methods here, to make sure the logic will always be consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cdap-proto/src/main/java/io/cdap/cdap/proto/operation/OperationRunStatus.java
@@ -48,6 +48,8 @@ public boolean canTransitionTo(OperationRunStatus status) {
return true;
}
switch (this) {
case PENDING:
return status == STARTING || status == STOPPING;
Copy link
Contributor

Choose a reason for hiding this comment

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

can it ever go directly to failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the failure message is only send for runtime/runner after STARTING message is processed.

@samdgupi samdgupi requested a review from albertshau November 13, 2023 19:00
albertshau
albertshau approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@albertshau albertshau left a comment

Choose a reason for hiding this comment

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

one comment about removing guava from proto

cdap-proto/pom.xml Outdated Show resolved Hide resolved
@samdgupi samdgupi merged commit d068939 into develop Nov 14, 2023
7 checks passed
@samdgupi samdgupi deleted the get-pending-runs branch November 14, 2023 08:39
@samdgupi samdgupi mentioned this pull request Nov 20, 2023
Merged
samdgupi added a commit that referenced this pull request Nov 20, 2023
@samdgupi
Merge pull request #15444 from cdapio/cherrypick-15424
ea3c7e9 
[🍒 ] Add pending run scan method from #15424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertshau albertshau albertshau approved these changes

@GnsP GnsP Awaiting requested review from GnsP

Assignees

@samdgupi samdgupi

Labels
6.10 build Triggers github actions build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samdgupi @albertshau