[HA] An empty replica breaks when asks for a full resync#2595
[HA] An empty replica breaks when asks for a full resync#2595lvca merged 5 commits intoArcadeData:mainfrom
Conversation
Summary of ChangesHello @carlos-rodrigues-8x8, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a bug affecting High Availability (HA) mode where an empty replica would encounter an error during a full resynchronization process. The primary fix involves a more robust method for retrieving file components, preventing failures in scenarios where files might not be immediately present. Additionally, a minor refinement was made to the server startup script for improved argument handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in High Availability mode and improves the server startup script. The change in PageManager.java makes the page flushing process more robust by using getFileByIdIfExists, which prevents exceptions when a file does not exist, a scenario that can occur during HA synchronization. The change in server.sh corrects the handling of command-line arguments by removing a redundant parameter that caused arguments to be duplicated. Both changes are correct and improve the codebase. I have one suggestion for the shell script to further improve its robustness regarding argument handling.
package/src/main/scripts/server.sh
Outdated
| $ARCADEDB_SETTINGS \ | ||
| -cp "$ARCADEDB_HOME/lib/*" \ | ||
| $ARGS $* "$@" com.arcadedb.server.ArcadeDBServer | ||
| $ARGS "$@" com.arcadedb.server.ArcadeDBServer |
There was a problem hiding this comment.
While removing the redundant $* is the correct fix for duplicated arguments, the use of unquoted $ARGS on this line can be fragile. If a user sets $ARGS to a value containing spaces that should be preserved within a single argument (e.g., a path), shell word-splitting will break it. For example, ARGS='-Dmy.prop="some value"' would be incorrectly passed to the Java process.
This is a common limitation in sh scripts. If it's not feasible to switch to bash and use arrays for robust argument handling, it would be beneficial to add a comment to the script documenting that variables like $ARGS should not contain values with spaces.
There was a problem hiding this comment.
Removed used variable
There was a problem hiding this comment.
We need to tightly review this change. I remember experimenting with that before and it led to problems.
There was a problem hiding this comment.
Which change @gramian? Removing $* or $ARGS?
There was a problem hiding this comment.
Both $* $@ are doing the same thing. Doesn't make sense to have both in the command. The slightly difference between both is $* expands to a single string containing all arguments separated by a space, while $@ is an array, but we are using it as string since it is between ". If you feel comfortable I can change to use $*.
There was a problem hiding this comment.
We can try, we just need to test if the various shells (including MacOS) support both, or if there are corner cases where it could break.
There was a problem hiding this comment.
Unfortunately I don't remember what the problem was, but I was using the same reasoning as you above.
package/src/main/scripts/server.sh
Outdated
| $ARCADEDB_SETTINGS \ | ||
| -cp "$ARCADEDB_HOME/lib/*" \ | ||
| $ARGS $* "$@" com.arcadedb.server.ArcadeDBServer | ||
| "$@" com.arcadedb.server.ArcadeDBServer |
There was a problem hiding this comment.
@robfrank this looks fine to me, not sure about this change in the script. WDYT?
There was a problem hiding this comment.
As I explained before to @gramian, both $* and $@ doing the same thing, we are just duplicating the parameters.
There was a problem hiding this comment.
Reverted! Should I open another ticket to handle it?
lvca
left a comment
There was a problem hiding this comment.
This is ok for me, not sure about the change in the server.sh
|
@carlos-rodrigues-8x8 could you please remove the change to server.sh? |
|
Thanks @carlos-rodrigues-8x8 ! |
* Adding fix for full sync request * Cosmetic patch to avoid duplication of configurations * Reverting deleted lines * Removing unused variable * Reverting server.sh (cherry picked from commit fc1d7a1)
What does this PR do?
This PR is meant to fix a bug on full synch request on HA mode, as described in the open issue.
Motivation
Contributing for evolving arcadedb.
Related issues
#2594
Additional Notes
Checklist
mvn clean packagecommand