-
Notifications
You must be signed in to change notification settings - Fork 92
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
MINIFICPP-1743 Added PutGCSObject processor #1268
Conversation
9dda1b7
to
c65e698
Compare
c65e698
to
f45b76f
Compare
extensions/gcp/controllerservices/GcpCredentialsControllerService.h
Outdated
Show resolved
Hide resolved
extensions/gcp/controllerservices/GcpCredentialsControllerService.h
Outdated
Show resolved
Hide resolved
extensions/gcp/GCPAttributes.h
Outdated
flow_file.setAttribute(GCS_CREATE_TIME_ATTR, std::to_string(object_metadata.time_created().time_since_epoch().count())); | ||
flow_file.setAttribute(GCS_UPDATE_TIME_ATTR, std::to_string(object_metadata.updated().time_since_epoch().count())); | ||
flow_file.setAttribute(GCS_DELETE_TIME_ATTR, std::to_string(object_metadata.time_deleted().time_since_epoch().count())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these in seconds, milliseconds, or something else? I think we should either write these timestamps in an unambiguous format like iso8601, or add something like ".unix-timestamp.seconds" to their name and put a duration_cast
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch it was ambiguous so I've added the duration_cast: 0492f5b#diff-458da9b99a0922017a48a3a4264bee04d171dce2845bce752aa2ea8430a0df26R59-R61
However about the name and type:
nifi uses the same naming scheme with simple integer timestamps, I think we should be consistent with that.
0492f5b#diff-458da9b99a0922017a48a3a4264bee04d171dce2845bce752aa2ea8430a0df26R59-R61
Maybe we should document this in PROCESSORS.md, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that makes sense. Yes, documenting the format of these timestamps in PROCESSORS.md (while rolling our eyes in the general direction of NiFi developers) is probably the best we can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included a table describing the attributes in PROCESSORS.md
https://github.com/apache/nifi-minifi-cpp/pull/1268/files#diff-fd2410931e7fdc4bf8b3ce23f5f7a27c7aacdf9337320626d86d806448c90b9bR1503
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, I missed that, sorry. I would prefer "(Unix timestamp in milliseconds)" instead of "(milliseconds)", but it's OK either way.
Co-authored-by: Ferenc Gerlits <fgerlits@users.noreply.github.com>
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?
For code changes:
For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.