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

Jump to bottom

1709723: last check-in date is not updated for hypervisors reported by virt-who (candlepin changes) [ENT-1049] #2314

Merged
merged 1 commit into from May 16, 2019
Merged

1709723: last check-in date is not updated for hypervisors reported by virt-who (candlepin changes) [ENT-1049] #2314

merged 1 commit into from May 16, 2019

Conversation

Januson
Copy link
Contributor

@Januson Januson commented Apr 30, 2019

  • Create a new endpoint POST /hypervisors/heartbeat/{owner}
  • The new endpoint updates last checkin date of all consumers for the given reporter_id

@nikosmoum nikosmoum self-assigned this Apr 30, 2019
barnabycourt
barnabycourt reviewed Apr 30, 2019
View reviewed changes
server/src/main/java/org/candlepin/model/ConsumerCurator.java Outdated
" SET lastcheckin = :checkin" +
" WHERE id IN (SELECT consumer_id" +
" FROM cp_consumer_hypervisor" +
" WHERE reporter_id = :reporter)";
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be scoped to the current organization.

Copy link
Member

Choose a reason for hiding this comment

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

Don't need a subselect:

        "UPDATE cp_consumer a, cp_consumer_hypervisor b, cp_owner c" +
        " SET a.lastcheckin = :checkin" +
        " WHERE a.id = b.consumer_id" +
        " AND b.reporter_id = :reporter" +
        " AND a.owner_id = c.id" +
        " AND c.account = :orgKey;"

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.

nikosmoum
nikosmoum requested changes May 2, 2019
View reviewed changes
Copy link
Member

@nikosmoum nikosmoum left a comment

Choose a reason for hiding this comment

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

A few changes required that shouldn't be a problem, before we can merge.

server/spec/hypervisor_heartbeat_spec.rb Outdated Show resolved Hide resolved
server/src/main/java/org/candlepin/pinsetter/tasks/HypervisorHeartbeatUpdateJob.java Outdated Show resolved Hide resolved
server/src/main/java/org/candlepin/resource/HypervisorResource.java Outdated Show resolved Hide resolved
server/src/main/java/org/candlepin/pinsetter/tasks/HypervisorHeartbeatUpdateJob.java Outdated
/**
* Asynchronous job for refreshing the lastCheckin for specific reporter_id.
* A job will wait for a running job of the same Owner to finish before
* beginning execution
Copy link
Member

Choose a reason for hiding this comment

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

Should probably remove the second sentence of the comment, since this job is not marked for uniqueness (it would have to either extend the UniqueByEntityJob class, or alternatively provide the isSchedulable & scheduleJob methods). Alternatively, you can make it unique for Owners & class (so that only one job of this type is running for this owner at a time), but I believe it is not necessary for this job.

@wottop Can you verify that we don't need this job to be unique-by-entity? ^

server/src/main/java/org/candlepin/pinsetter/tasks/HypervisorHeartbeatUpdateJob.java Show resolved Hide resolved
wottop
wottop requested changes May 2, 2019
View reviewed changes
server/src/main/java/org/candlepin/model/ConsumerCurator.java Outdated
" SET lastcheckin = :checkin" +
" WHERE id IN (SELECT consumer_id" +
" FROM cp_consumer_hypervisor" +
" WHERE reporter_id = :reporter)";
Copy link
Member

Choose a reason for hiding this comment

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

Don't need a subselect:

        "UPDATE cp_consumer a, cp_consumer_hypervisor b, cp_owner c" +
        " SET a.lastcheckin = :checkin" +
        " WHERE a.id = b.consumer_id" +
        " AND b.reporter_id = :reporter" +
        " AND a.owner_id = c.id" +
        " AND c.account = :orgKey;"

@Januson Januson force-pushed the ojanus/hearthbeat branch 4 times, most recently from 51b0c6f to cab60be Compare May 14, 2019 08:16
@Januson Januson changed the title 1652549: last check-in date is not updated for hypervisors reported by virt-who (candlepin changes) [ENT-1049] 1709723: last check-in date is not updated for hypervisors reported by virt-who (candlepin changes) [ENT-1049] May 14, 2019
@Januson Januson force-pushed the ojanus/hearthbeat branch 2 times, most recently from b219d0c to 84be24e Compare May 14, 2019 09:31
nikosmoum
nikosmoum requested changes May 14, 2019
View reviewed changes
Copy link
Member

@nikosmoum nikosmoum left a comment

Choose a reason for hiding this comment

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

Re-reviewing: the changes noted before are now fixed, and the code in general is good, but I missed looking at the API design itself before.

server/src/main/java/org/candlepin/resource/HypervisorResource.java Outdated
@POST
@Produces(MediaType.APPLICATION_JSON)
@Transactional
@Path("/heartbeat/{owner}")
Copy link
Member

Choose a reason for hiding this comment

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

Something I did not think about before is the design of the API itself.

  • For the verb: I would say that this should be either a PATCH or a PUT instead of a POST (technically, it fits much more to use a PATCH, since we don't accept an entire entity, like a ConsumerDTO or a HypervisorDTO to be updated, but we only want to update a specific field of the entity, the last check in date).

  • For the url: I think the {owner} should be at the start, since it is the top-level entity, which is consistent with all other endpoints we have. Also, maybe the reporter_id should also be in the URL, since this is what identifies the entity/ies being PATCHed. So the url should be one of: /{owner}/heartbeat, /{owner}/{reporter_id}/heartbeat or /{owner}/heartbeat/{reporter_id}.

@wottop @Ceiu @Januson What are your thoughts?

Copy link
Member

@wottop wottop May 15, 2019 •
edited

Choose a reason for hiding this comment

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

/{owner}/heartbeat and leave the reporter_id as a parameter like the hypervisor check in

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.

@Januson Januson force-pushed the ojanus/hearthbeat branch 2 times, most recently from 40b0f06 to 7691541 Compare May 15, 2019 14:20
nikosmoum
nikosmoum approved these changes May 15, 2019
View reviewed changes
Copy link
Member

@nikosmoum nikosmoum left a comment

Choose a reason for hiding this comment

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

ACK by me.

@Januson
1709723: last check-in date is not updated for hypervisors reported b…
2f310f2 
…y virt-who

- Create a new endpoint POST /hypervisors/{owner}/heartbeat
- The new endpoint updates last checkin date of all consumers for the given reporter_id
@nikosmoum nikosmoum merged commit 9ca027b into candlepin:master May 16, 2019
@Januson Januson deleted the ojanus/hearthbeat branch September 27, 2019 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barnabycourt barnabycourt barnabycourt left review comments

@wottop wottop wottop approved these changes

@nikosmoum nikosmoum nikosmoum approved these changes

Assignees

@nikosmoum nikosmoum

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@Januson @wottop @nikosmoum @barnabycourt