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.

Sign up for GitHub

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

Jump to bottom

Improve logging for catalog errors #11727

Merged
merged 1 commit into from Jul 8, 2020
Merged

Improve logging for catalog errors #11727

merged 1 commit into from Jul 8, 2020

Conversation

gpang
Copy link
Contributor

@gpang gpang commented Jul 7, 2020

No description provided.

@alluxio-bot
Copy link
Contributor

Automated checks report:

  • AmplabJenkins build check: PENDING
    • We were not able to detect AmplabJenkins test results on this PR. Status will update when testing completes.
  • Commits associated with Github account: PASS
  • PR title follows the conventions: PASS

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

HelloHorizon
HelloHorizon reviewed Jul 7, 2020
View reviewed changes
Copy link
Contributor

@HelloHorizon HelloHorizon left a comment

Choose a reason for hiding this comment

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

@gpang Thanks gene! I leave some minor comments.

table/server/common/src/main/java/alluxio/master/table/DatabaseInfo.java
@@ -47,7 +49,11 @@ public DatabaseInfo(String location, String ownerName, PrincipalType ownerType,
mOwnerName = ownerName;
mOwnerType = ownerType;
mComment = comment;
mParameters = params;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are location and comment also need a non-null value? Glue doesn't require user to set values for these fields in the database?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but those fields were already annotated to be @Nullable so I am assuming it is ok to be null.

table/server/common/src/main/java/alluxio/master/table/DatabaseInfo.java
mParameters = Collections.emptyMap();
} else {
mParameters = params;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The glue UDB handles the NPE for parameters, do we want to check it again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I put that here, was because this field was not classified as @Nullable, so I am enforcing that here.

gpang
gpang commented Jul 7, 2020
View reviewed changes
Copy link
Contributor Author

@gpang gpang left a comment

Choose a reason for hiding this comment

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

@HelloHorizon

table/server/common/src/main/java/alluxio/master/table/DatabaseInfo.java
@@ -47,7 +49,11 @@ public DatabaseInfo(String location, String ownerName, PrincipalType ownerType,
mOwnerName = ownerName;
mOwnerType = ownerType;
mComment = comment;
mParameters = params;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but those fields were already annotated to be @Nullable so I am assuming it is ok to be null.

table/server/common/src/main/java/alluxio/master/table/DatabaseInfo.java
mParameters = Collections.emptyMap();
} else {
mParameters = params;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I put that here, was because this field was not classified as @Nullable, so I am enforcing that here.

HelloHorizon
HelloHorizon approved these changes Jul 7, 2020
View reviewed changes
Copy link
Contributor

@HelloHorizon HelloHorizon left a comment

Choose a reason for hiding this comment

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

LGTM

@AmplabJenkins
Copy link

Merged build finished. Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-PR-Builder/10381/
Test FAILed.

@gpang
Copy link
Contributor Author

gpang commented Jul 7, 2020

Jenkins, test this please

@AmplabJenkins
Copy link

Merged build finished. Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-PR-Builder/10383/
Test FAILed.

@HelloHorizon
Copy link
Contributor

Jenkins, test this please

@AmplabJenkins
Copy link

Merged build finished. Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-PR-Builder/10384/
Test FAILed.

@gpang
Copy link
Contributor Author

gpang commented Jul 7, 2020

alluxio-bot, test this please

@alluxio-bot
Copy link
Contributor

alluxio-bot triggering jenkins retest: 0/3

@alluxio-bot
Copy link
Contributor

jenkins, test this please

@AmplabJenkins
Copy link

Merged build finished. Test FAILed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-PR-Builder/10386/
Test FAILed.

@alluxio-bot
Copy link
Contributor

alluxio-bot triggering jenkins retest: 1/3

@alluxio-bot
Copy link
Contributor

jenkins, test this please

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-PR-Builder/10388/
Test PASSed.

@gpang
Copy link
Contributor Author

gpang commented Jul 8, 2020

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit b8cca5b into Alluxio:master Jul 8, 2020
@gpang
Copy link
Contributor Author

gpang commented Jul 8, 2020

alluxio-bot, cherry-pick this to branch-2.3 please

@alluxio-bot alluxio-bot mentioned this pull request Jul 8, 2020
Merged
@alluxio-bot
Copy link
Contributor

Auto cherry-pick to branch branch-2.3 successfully opened PR: #11730

alluxio-bot pushed a commit that referenced this pull request Jul 8, 2020
@gpang @alluxio-bot
Improve logging for catalog errors
7309321 
pr-link: #11727
change-id: cid-a83e08d7c28a950e7c6618c7515c2a10681a7942
alluxio-bot added a commit that referenced this pull request Jul 8, 2020
@alluxio-bot
Improve logging for catalog errors
c0a805a 
Cherry-pick of existing commit.
orig-pr: #11727
orig-commit: b8cca5b
orig-commit-author: Gene Pang <gene.pang@gmail.com>

pr-link: #11730
change-id: cid-a83e08d7c28a950e7c6618c7515c2a10681a7942
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HelloHorizon HelloHorizon HelloHorizon approved these changes

Assignees

@ZacBlanco ZacBlanco

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gpang @alluxio-bot @AmplabJenkins @HelloHorizon @ZacBlanco