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

Cleanup inconsistent use of logging strings #2618

Closed
t92549 opened this issue Mar 24, 2022 · 1 comment · Fixed by #2651
Closed

Cleanup inconsistent use of logging strings #2618

t92549 opened this issue Mar 24, 2022 · 1 comment · Fixed by #2651
Labels
enhancement Improvement to existing functionality/feature good first issue Small, lower complexity and doesn't require pre-existing Gaffer knowledge tech-debt Relates to Technical Debt
Milestone
v1.23.0

Comments

@t92549
Copy link
Contributor

t92549 commented Mar 24, 2022

In many places in Gaffer, correct logging string formatting is used:

Gaffer/store-implementation/accumulo-store/src/main/java/uk/gov/gchq/gaffer/accumulostore/operation/hdfs/handler/AddElementsFromHdfsHandler.java

Line 254 in fb63fc2

LOGGER.info("Ensuring output directory {} doesn't exist", outputPath);

However, in some places, string concatenation is incorrectly used instead:

Gaffer/store-implementation/accumulo-store/src/main/java/uk/gov/gchq/gaffer/accumulostore/operation/hdfs/handler/AddElementsFromHdfsHandler.java

Line 66 in fb63fc2

LOGGER.info("Using working directory for splits files: " + splitsFilePath);

Although concatenation works, it is considered bad practice as the concatenated string object is always created, whether or not that string will be logged (based on the logging level). However, with the former method of formatting, the object is only created if that string will be logged. This means better performance.
As well as that I find the formatting easier to read.
@t92549 t92549 added tech-debt Relates to Technical Debt good first issue Small, lower complexity and doesn't require pre-existing Gaffer knowledge labels Mar 24, 2022
@t92549 t92549 added this to the v2_backlog milestone Mar 24, 2022
@t92549 t92549 mentioned this issue Mar 24, 2022
Merged
@RobertG111
Copy link
Contributor

Hi. I have made a pull request regarding this issue #2651. Thanks.

@t92549 t92549 linked a pull request May 20, 2022 that will close this issue
gh-2618: Cleanup inconsistent use of logging strings #2651
Merged
@t92549 t92549 mentioned this issue May 20, 2022
Merged
@n3101 n3101 modified the milestones: v2_backlog, v1.23.0 May 24, 2022
t92549 pushed a commit that referenced this issue May 26, 2022
@RobertG111
gh-2618: Cleanup inconsistent use of logging strings (#2651)
d354dc6 
* Found 1 extra inconsistency than the mentioned one

* found and fixed other inconsitencies

* further fixes

* further fixes 2

* fixed 3 out of 4 checkstyle errors - 1 more to fix

* all checkstyle errors fixed

* removed use of String.format

* removed another instance of using String.format

* more changes

* additional change

* reverted changes
t92549 pushed a commit that referenced this issue May 26, 2022
@RobertG111 @t92549
gh-2618: Cleanup inconsistent use of logging strings (#2651)
4efa8b8 
* Found 1 extra inconsistency than the mentioned one

* found and fixed other inconsitencies

* further fixes

* further fixes 2

* fixed 3 out of 4 checkstyle errors - 1 more to fix

* all checkstyle errors fixed

* removed use of String.format

* removed another instance of using String.format

* more changes

* additional change

* reverted changes
t92549 added a commit that referenced this issue May 27, 2022
@t92549 @RobertG111
gh-2657: Cherry-pick gh-2618: Cleanup inconsistent use of logging str…
f217c92 
…ings (#2651) (#2658)

* gh-2618: Cleanup inconsistent use of logging strings (#2651)

* Found 1 extra inconsistency than the mentioned one

* found and fixed other inconsitencies

* further fixes

* further fixes 2

* fixed 3 out of 4 checkstyle errors - 1 more to fix

* all checkstyle errors fixed

* removed use of String.format

* removed another instance of using String.format

* more changes

* additional change

* reverted changes

* gh-2657: Revert accidental change

* gh-2657: Update copyright with spotless

Co-authored-by: Robert <33911814+RobertG111@users.noreply.github.com>
@t92549 t92549 added the enhancement Improvement to existing functionality/feature label Jun 6, 2022
t92549 added a commit that referenced this issue May 12, 2023
@t92549 @GCHQDeveloper314
@p3430233 @r32575 @RobertG111 @AnirbanPatragithub @dependabot @kvaithin @ibrijesh
Gh 2940: Merge develop into v2-alpha (#2945)
1bbd1dd 
* Update docs links in README

* Revert "Update docs links in README"

This reverts commit 2d87bf3.

* gh-2559 added hasTrait, its handler and corresponding tests (#2561)

* gh-2559 added hasTrait, its handler and corresponding tests

* gh-2559 amended comment in HasTrait, added additional tests for the handler

* gh-2559 null check & tidy test

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>

* Update links to Gaffer docs in all files (#2566)

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>

* gh-2604: Deprecates Parquet (#2606)

* gh-2556: Add HBaseStore deprecated tag, javadocs and note to README (#2605)

* gh-2556: Add HBaseStore deprecated tag, javadocs and note to README

* gh-2556: Amend version number

* Upgrade Hazelcast version to 5.1.1 (#2612)

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>

* Gh-2602: Replace Log4j with Reload4j (#2609)

* Replace Log4j with Reload4j
Also add enforcer plugin to prevent future problems

* FederatedStoreGetTraitsTest did not delete the cache service before carrying out each test causing it to fail when run as part of the maven tests. In isolation it passed. Found that the FederatedGraphStorageTraitsTest was also not clearing the cache. Added the before and after cache clearance and bumped the JUnit version of FederatedStoreGetTraitsTest from 4 to 5. Tests now pass (#2617)

Co-authored-by: r32575 <63105662+r32575@users.noreply.github.com>

* Gh 2562 GetElementsWithinSet not allowing for the exclusion of properties - Gaffer v1 (#2597)

* Fixed bug gh-2562. View properties are now being filtered. Added tests

* After a review moved the return statements inside the doPostFilter if-check and changed a conditional fail to an assertThat test

* Removed the try-finally blocks as the classes implement CloseableIterable

* Added failure message to test

* Adjusted javadoc formatting

* Removed rogue letter

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>

* prepare release gaffer2-1.22.0

* prepare for next development iteration

* gh-2618: Cleanup inconsistent use of logging strings (#2651)

* Found 1 extra inconsistency than the mentioned one

* found and fixed other inconsitencies

* further fixes

* further fixes 2

* fixed 3 out of 4 checkstyle errors - 1 more to fix

* all checkstyle errors fixed

* removed use of String.format

* removed another instance of using String.format

* more changes

* additional change

* reverted changes

* replaced to SLF4J (#2663)

* gh-2669: Added new aggregation function endpoints (#2670)

* gh-2673: Fix links to koryphe core (#2674)

* Gh 2705 operation details spring (#2706)

* gh-2705: Cherry pick new spring endpoint

* gh-2696: Added tests for new all operation details endpoint (#2700)

* gh-2696: Added tests for new all operation details endpoint

* gh-2696: Spotless apply

* gh-2705: Copyright fix

* Gh-2745: Fix crown copyright link

Fixed Crown Copyright Link on Line 44

* Gh-2776: Create issue templates (#2777)

* Create issue templates

* Simplify bug report

* build(deps): bump hazelcast (#2848)

Bumps [hazelcast](https://github.com/hazelcast/hazelcast) from 5.1.1 to 5.1.3.
- [Release notes](https://github.com/hazelcast/hazelcast/releases)
- [Commits](hazelcast/hazelcast@v5.1.1...v5.1.3)

---
updated-dependencies:
- dependency-name: com.hazelcast:hazelcast
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* GH-2845: updt class var parameter to use Map interface (#2846)

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>

* Gh-2914: Remove redundant constants class (#2927)

* #2914 Issue: Remove redundant constants class

* Simplify charset

* Fix import

* Remove erroneous use of charset with JSONSerialiser.serialise

* Use StandardCharsets inbuilt function
---------

Co-authored-by: GCHQDeveloper314 <94527357+GCHQDeveloper314@users.noreply.github.com>

* prepare release gaffer2-1.23.0

* prepare for next development iteration

* Fix field names in ToCsv

* Refactor to string enums

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: GCHQDeveloper314 <94527357+GCHQDeveloper314@users.noreply.github.com>
Co-authored-by: p3430233 <61738524+p3430233@users.noreply.github.com>
Co-authored-by: r32575 <63105662+r32575@users.noreply.github.com>
Co-authored-by: Gaffer <github-actions@github.com>
Co-authored-by: Robert <33911814+RobertG111@users.noreply.github.com>
Co-authored-by: Anirban Patra <96457415+AnirbanPatragithub@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: kvaithin <68995267+kvaithin@users.noreply.github.com>
Co-authored-by: Brijesh Yadav <brijeshyadavjthj@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Improvement to existing functionality/feature good first issue Small, lower complexity and doesn't require pre-existing Gaffer knowledge tech-debt Relates to Technical Debt
Projects
None yet
Milestone
v1.23.0
Development

Successfully merging a pull request may close this issue.

gh-2618: Cleanup inconsistent use of logging strings
3 participants
@RobertG111 @n3101 @t92549