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

Add a GQLSTATUS code to the current errors #176

Closed
wants to merge 1 commit into from

Conversation

renetapopova
Copy link
Collaborator

Hey @Lojjs, please take a look to see if that makes sense for the current errors.

@neo-technology-commit-status-publisher
Copy link
Collaborator

Thanks for the documentation updates.

The preview documentation has now been torn down - reopening this PR will republish it.

Copy link
Contributor

@Lojjs Lojjs left a comment

Choose a reason for hiding this comment

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

I think the general format would work, if we just make sure that is covering everything and is kept up-to-date (which I think will be a challenge) and make it clearer which GQLSTATUS codes are top-level vs. cause

= List of all server error codes
= List of all Neo4j 5 server error codes

Error codes are Neo4j status codes returned by the server when the execution of a query fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is always hard with naming, but I wonder if 'error codes' is too unspecific here? The new GQLSTATUS codes are also a type of error codes is my thinking.

Maybe the other way around i.e. that we call the section
'List of all Neo4j 5 status codes' and rewrite this sentence to 'Neo4j status codes are error codes returned by the server...'. Let me know what you think

Copy link
Collaborator Author

@renetapopova renetapopova Sep 13, 2024

Choose a reason for hiding this comment

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

I somehow needed to differentiate between both pages. We'll have another page about the GQLSTATUS codes. So, in the end, the structure will be something similar to:

  • Server errors
    • List of all Neo4j 5 server error codes
    • List of all GQLSTATUS server error codes
  • Server notifications


Client errors::
These errors are caused by the client and are usually related to the request itself.
Client errors have the prefix `Neo.ClientError`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just moved text, but we could maybe clarify it by writing 'Neo4j status codes for client errors have the prefix Neo.ClientError'. Same for transient errors and database errors too

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 am not sure what you're suggesting. The whole page is about Neo4j status codes only.

@@ -4,18 +4,4 @@
= Server errors
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we might also want to rewrite the rest of the text here a bit and turn this into more of an overview page? That can be a separate PR though

|===
|Description
a| Invalid change identifier.
|GQLSTATUS code(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should clarify that this is a cause GQLSTATUS code in some way. In the next table you have the top level code in the same place and that distinction would be important to know for someone adapting their code to the new system

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, this shouldn't be the cause code. This is the top-level code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

! Message ! GQLSTATUS code
! Invalid change identifier. ! 52N26
! Given change identifier does not belong to this database ! 52N31
! Given ChangeIdentifier describes a transaction that hasn't yet occurred ! 52N30
Copy link
Contributor

Choose a reason for hiding this comment

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

When I look at the CDC tab in the Error spreadsheet the one listed as 52N30 here (CDC-005) is actually 52N32 (although it has an old comment from me and the same old message as CDC-001 which is 52N30). Also CDC-004 seems to be missing.

I am not sure which source you have used for the table, so it can be either a misstake there or by you. The rest of the codes in this table look correct

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 think we are seeing different things in the spreadsheet. This is what I see for 52N16:

Screenshot 2024-09-13 at 12 09 25

This error occurs when the request cannot be processed by this server.
Write requests can only be processed by the leader.

.Error details
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe that is because this is just a first draft, but the messages below are only a subset of the ones for Neo.ClientError.Cluster.NotALeader

@renetapopova
Copy link
Collaborator Author

We decided to go with a different solution.

@renetapopova renetapopova added wontfix This will not be worked on and removed DO NOT MERGE 5.25 labels Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants