Copilot commented on code in PR #95:
URL:
https://github.com/apache/skywalking-banyandb-java-client/pull/95#discussion_r2323934581
##########
src/main/java/org/apache/skywalking/banyandb/v1/client/StreamWrite.java:
##########
@@ -68,7 +65,14 @@ public void setTimestamp(long timestamp) {
* @return {@link BanyandbStream.WriteRequest} for the bulk process.
*/
@Override
- protected BanyandbStream.WriteRequest build(BanyandbCommon.Metadata
metadata, Timestamp ts) {
+ protected BanyandbStream.WriteRequest build(BanyandbCommon.Metadata
metadata) {
+ if (!timestamp.isPresent() || timestamp.get() <= 0) {
+ throw new IllegalArgumentException("timestamp is invalid.");
+ }
+ Timestamp ts = Timestamp.newBuilder()
+ .setSeconds(timestamp.get() / 1000)
+ .setNanos((int) (timestamp.get() % 1000 * 1_000_000)).build();
Review Comment:
This timestamp conversion logic is duplicated between StreamWrite and
MeasureWrite. Consider extracting it to a utility method in AbstractWrite to
avoid code duplication.
##########
src/main/java/org/apache/skywalking/banyandb/v1/client/MeasureWrite.java:
##########
@@ -58,7 +58,14 @@ public MeasureWrite tag(String tagName,
Serializable<BanyandbModel.TagValue> tag
* @return {@link BanyandbMeasure.WriteRequest} for the bulk process.
*/
@Override
- protected BanyandbMeasure.WriteRequest build(BanyandbCommon.Metadata
metadata, Timestamp ts) {
+ protected BanyandbMeasure.WriteRequest build(BanyandbCommon.Metadata
metadata) {
+ if (!timestamp.isPresent() || timestamp.get() <= 0) {
+ throw new IllegalArgumentException("timestamp is invalid.");
Review Comment:
The error message should be more specific about what constitutes a valid
timestamp. Consider: 'Timestamp is required and must be greater than 0 for
measure writes.'
```suggestion
throw new IllegalArgumentException("Timestamp is required and
must be greater than 0 for measure writes.");
```
##########
src/main/java/org/apache/skywalking/banyandb/v1/client/StreamWrite.java:
##########
@@ -68,7 +65,14 @@ public void setTimestamp(long timestamp) {
* @return {@link BanyandbStream.WriteRequest} for the bulk process.
*/
@Override
- protected BanyandbStream.WriteRequest build(BanyandbCommon.Metadata
metadata, Timestamp ts) {
+ protected BanyandbStream.WriteRequest build(BanyandbCommon.Metadata
metadata) {
+ if (!timestamp.isPresent() || timestamp.get() <= 0) {
+ throw new IllegalArgumentException("timestamp is invalid.");
Review Comment:
The error message should be more specific about what constitutes a valid
timestamp. Consider: 'Timestamp is required and must be greater than 0 for
stream writes.'
```suggestion
throw new IllegalArgumentException("Timestamp is required and
must be greater than 0 for stream writes.");
```
##########
src/main/java/org/apache/skywalking/banyandb/v1/client/AbstractWrite.java:
##########
@@ -35,28 +33,31 @@ public abstract class AbstractWrite<P extends
com.google.protobuf.GeneratedMessa
/**
* Timestamp represents the time of current stream
* in the timeunit of milliseconds.
+ *
+ * Measure and stream require timestamp.
+ * Trace doesn't require extra timestamp.
Review Comment:
The timestamp field should have a more descriptive comment explaining when
it should be present (for Stream/Measure writes) versus when it's not needed
(for Trace writes).
```suggestion
* Timestamp represents the time of the current data point, in
milliseconds.
* <p>
* <b>When to set:</b>
* <ul>
* <li><b>Stream and Measure writes:</b> This field <i>must</i> be set
to indicate the event time.</li>
* <li><b>Trace writes:</b> This field is <i>not needed</i> and should
be left unset; trace data does not require an explicit timestamp here.</li>
* </ul>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
