Add task create & lease/acknowledge sample. #1153
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
bd33182 to
a8ea5c3
Compare
|
CLAs look good, thanks! |
| .build(); | ||
|
|
||
| public static void main(String... args) throws Exception { | ||
| // [START cloud_tasks_appengine_create_task] |
There was a problem hiding this comment.
Are you sure that you want the region tag to start here? It feels like most of the command line parsing isn't relevant enough that we want to link to it from elsewhere.
There was a problem hiding this comment.
I wasn't sure, but I also wanted to include enough context to understand what was happening. I don't know if these tags are used in the documentation, I was just copying them from the python sample. But I don't mind moving it.
| @Override | ||
| public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException { | ||
| log.info("Received task request: " + req.getServletPath()); | ||
| if (req.getParameter("payload") != null) { |
There was a problem hiding this comment.
It might be a good idea to add a line of logging for when the payload is null - that way if the user forgets to set a payload the endpoint will still logged that it was triggered.
There was a problem hiding this comment.
These log statements were actually just for my debugging purposes initially, but yeah I can add one for a null payload. Warning level probably, since it doesn't affect the execution?
There was a problem hiding this comment.
Typically logging is debug < info < warning < critical, so using ' debug' or 'info' is probably typical (but I don't think it makes a big difference for a smaller application).
tasks/pom.xml
Outdated
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <version>3.2</version> | ||
| </plugin> | ||
| <plugin> |
There was a problem hiding this comment.
Is building the jar with dependencies required for deployment? If not, can we remove this section?
| // [START cloud_tasks_lease_and_acknowledge_task] | ||
| private static void pullAndAckTask(String projectId, String queueName, String location) { | ||
| try (CloudTasksClient client = CloudTasksClient.create()) { | ||
| LeaseTasksRequest.Builder reqBuilder = LeaseTasksRequest.newBuilder() |
There was a problem hiding this comment.
Any reason you are passing a builder instead of just the request?
There was a problem hiding this comment.
I'm not, on line 148 it passes reqBuilder.build()
There was a problem hiding this comment.
Or do you mean declaring it locally as a request instead of a builder? If I'm taking the Ack request out into a var below, I'll do the same here for consistency.
There was a problem hiding this comment.
Yup that's what I meant - typically the builder pattern is a replacement for new, so it seems weird to trigger the build there. Looks good now.
| } | ||
| Task task = response.getTasksList().get(0); | ||
| System.out.println("Leased task: " + task.getName()); | ||
| client.acknowledgeTask(AcknowledgeTaskRequest |
There was a problem hiding this comment.
What do you think about building the AcknowledgeTaskRequest before the function and passing it in? I think it increases readability and makes it more obvious what the function requires.
|
Requested changes done. |
* Add task create & lease/acknowledge sample. * Add Appengine Queue Tasks sample.
* Add task create & lease/acknowledge sample. * Add Appengine Queue Tasks sample.
Still working on the appengine version, but thought I would create the pull request now so reviews can start, since we were wanting this done by EOD.
I do not have a README currently because the content seems copied from the documentation at https://cloud.google.com/tasks/docs/quickstart-pull, which someone else will be working on this weekend. I will grab it from there and pull it down when it is done.