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

Troubleshooting Guide #176

Merged

Conversation

prashastia
Copy link
Collaborator

/gcbrun

@@ -264,7 +264,7 @@ static String timestampRestrictionFromPartitionType(
// extract a datetime from the value and restrict
// between previous and next hour
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know support for unbounded read is going to be removed. But this was a very simple and easy change to fix reading of Time Partitions based on DAY, MONTH and YEAR.

TROUBLESHOOT.md Outdated Show resolved Hide resolved
TROUBLESHOOT.md Outdated Show resolved Hide resolved
TROUBLESHOOT.md Show resolved Hide resolved
TROUBLESHOOT.md Outdated
Comment on lines 79 to 81
- It is also expected that the value passed in the Avro Generic Record follows the Schema.
Here the “records passed” indicates the modified records after passing through the series of
subtasks defined in the application pipeline.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

This info is well captured in preceding and next points

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Value in the avro record might differ from the avro schema of the Generic Record (since Flink Does not impose any check on the value of the field) This is the error faced by GMF very early on in testing the connector when they accidentally passed the wrong value INTEGER in an ARRAY type field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is a problem where we must prompt the user to ensure that their destination table matches with the schema of records received by the sink.

The statement we should remove is ..

Here the “records passed” indicates the modified records after passing through the series of 
subtasks defined in the application pipeline.

.. because this doesn't prompt a schema check, instead says something fairly obvious that doesn't add much value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair, removed.

TROUBLESHOOT.md Outdated Show resolved Hide resolved
TROUBLESHOOT.md Outdated Show resolved Hide resolved
TROUBLESHOOT.md Outdated Show resolved Hide resolved
@prashastia prashastia mentioned this pull request Oct 29, 2024
@prashastia prashastia force-pushed the troubleshooting-guide branch from 89b1e1a to 89050de Compare November 12, 2024 11:35

public AvroDeserializationSchema(String avroSchemaString) {
this.avroSchemaString = avroSchemaString;
}

@Override
public GenericRecord deserialize(GenericRecord record) throws IOException {
public GenericRecord deserialize(GenericRecord record) throws BigQueryConnectorException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You dont need to throw any exception in this method's definition

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed.

Comment on lines 64 to 74
try {
if (deserialize != null) {
out.collect(deserialize);
}
} catch (Exception e) {
LOG.error(
String.format(
"Failed to forward the deserialized record %s to the next operator.%nError %s%nCause %s",
deserialize, e.getMessage(), e.getCause()));
throw new BigQueryConnectorException(
"Failed to forward the deserialized record to the next operator.", e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reduce nesting as much as you can.
How about:

        if (deserialize == null) {
            return;
        }
        try {
            out.collect(deserialize);
        } catch (Exception e) {
        ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

Comment on lines 177 to 181
// Reset the "Since Checkpoint" values to 0.
numberOfRecordsBufferedByBigQuerySinceCheckpoint.dec(
numberOfRecordsBufferedByBigQuerySinceCheckpoint.getCount());
numberOfRecordsSeenByWriterSinceCheckpoint.dec(
numberOfRecordsSeenByWriterSinceCheckpoint.getCount());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is being tracked in a separate PR. Please remove from here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

TROUBLESHOOT.md Outdated
Comment on lines 40 to 42
- The problem lies with the pipeline, the previous chain of subtasks that are performed before
sink is called.
- The pipeline is not processing and passing the records forward for the sink.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most likely not an issue in the sink, since previous subtasks are not passing records forward for the sink.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

TROUBLESHOOT.md Outdated
#### The records are arriving at the sink but not being successfully written to BigQuery.
Check the logs or error message for the following errors:
#### `BigQuerySerializationException`
- This message illustrates that the record(s) could not be serialized by the connector.
Copy link
Collaborator

Choose a reason for hiding this comment

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

record not record(s) since serialize exception for every record will be logged individually

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

TROUBLESHOOT.md Outdated
Check the logs or error message for the following errors:
#### `BigQuerySerializationException`
- This message illustrates that the record(s) could not be serialized by the connector.
- The error message would also contain the actual cause for the same.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed

TROUBLESHOOT.md Outdated
Comment on lines 49 to 50
- Note: This error is not thrown but logged,
indicating that the connector was "Unable to serialize record" due to this error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

- This error is logged not thrown, explaining why the record could not be serialized.
- In future, this will be supplemented with dead letter queues.

Also, please mention logged not thrown in bold.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

TROUBLESHOOT.md Outdated
Comment on lines 79 to 81
- It is also expected that the value passed in the Avro Generic Record follows the Schema.
Here the “records passed” indicates the modified records after passing through the series of
subtasks defined in the application pipeline.
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is a problem where we must prompt the user to ensure that their destination table matches with the schema of records received by the sink.

The statement we should remove is ..

Here the “records passed” indicates the modified records after passing through the series of 
subtasks defined in the application pipeline.

.. because this doesn't prompt a schema check, instead says something fairly obvious that doesn't add much value

@prashastia
Copy link
Collaborator Author

@clmccart Pls review this PR. Thanks!

@jayehwhyehentee jayehwhyehentee self-requested a review November 19, 2024 11:49
Copy link
Collaborator

@jayehwhyehentee jayehwhyehentee left a comment

Choose a reason for hiding this comment

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

LGTM. Please merge after @clmccart's approval

Copy link
Contributor

@clmccart clmccart left a comment

Choose a reason for hiding this comment

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

are the changes that arent the troubleshooting guide supposed to be included in this PR?

TROUBLESHOOT.md Outdated
### Records are not being written to BigQuery
With the help of metrics available as a part of 0.4.0 release of the connector,
users should be able to track the number of records that enter the sink(writer) and the
number of records successfully written to BigQuery.
Copy link
Contributor

Choose a reason for hiding this comment

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

lets reference the two metric names here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

users should not face this error.
Users might face this error in case custom serializer is used.

## Known Issues/Limitations
Copy link
Contributor

Choose a reason for hiding this comment

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

let's also reference the 100 max parallelism here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have mentioned this in the Readme, should we mention it here as well?

@prashastia
Copy link
Collaborator Author

prashastia commented Dec 8, 2024

are the changes that aren't the troubleshooting guide supposed to be included in this PR?

@clmccart Yep, some error messages documented in the troubleshooting guide and minor bugs are fixed as a part of these PR as well.

@prashastia prashastia requested a review from clmccart December 10, 2024 05:29
@jayehwhyehentee jayehwhyehentee merged commit 8d519aa into GoogleCloudDataproc:main Dec 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants